{"thread":{"id":"45281","subject":"bisect-helper: we do not bisect --objects","startedAt":"2017-03-03T16:34:52Z","lastAt":"2017-03-06T20:28:55Z","messageCount":7,"participants":["Junio C Hamano","Christian Couder","Philip Oakley"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"313192","messageId":"xmqqvarq9vzo.fsf@gitster.mtv.corp.google.com","threadId":"45281","inReplyTo":null,"subject":"bisect-helper: we do not bisect --objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T16:16:11Z","receivedAt":"2017-03-03T16:34:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ever since \"bisect--helper\" was introduced in 1bf072e366\n(\"bisect--helper: implement \"git bisect--helper\"\", 2009-03-26),\nafter setting up the \"rev-list $bad --not $good_ones\" machinery, the\ncode somehow prepared to mark the trees and blobs at the good boundary\nas uninteresting, only when --objects option was given. This was kept\nacross a bit of refactoring done by 2ace9727be (\"bisect: move common\nbisect functionality to \"bisect_common\"\", 2009-04-19) and survives\nto this day.\n\nHowever, \"git bisect\" does not care about tree/blob object\nreachability at all---it purely works at the commit DAG level and\nnobody passes (and nobody should pass) \"--objects\" option to the\nunderlying rev-list machinery. Remove the dead code.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Christian, do you recall what we were thinking when we added this\n mark_edges_uninteresting() call in this program? If you don't,\n don't worry--this was done more than 8 years ago. I am just\n being curious and also a bit being cautious in case I am missing\n something.\n\n Thanks.\n\n bisect.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 30808cadf7..86c5929a23 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)\n {\n \tif (prepare_revision_walk(revs))\n \t\tdie(\"revision walk setup failed\");\n-\tif (revs->tree_objects)\n-\t\tmark_edges_uninteresting(revs, NULL);\n }\n \n static void exit_if_skipped_commits(struct commit_list *tried,\n"},{"id":"313245","messageId":"CAP8UFD0tzH+QiNM2BMhe9qcKdD0rP9T=Ry94=EPV+=d+7cdLfQ@mail.gmail.com","threadId":"45281","inReplyTo":"xmqqvarq9vzo.fsf@gitster.mtv.corp.google.com","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-03-03T23:04:54Z","receivedAt":"2017-03-03T23:13:02Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 3, 2017 at 5:16 PM, Junio C Hamano wrote:\n> Ever since \"bisect--helper\" was introduced in 1bf072e366\n> (\"bisect--helper: implement \"git bisect--helper\"\", 2009-03-26),\n> after setting up the \"rev-list $bad --not $good_ones\" machinery, the\n> code somehow prepared to mark the trees and blobs at the good boundary\n> as uninteresting, only when --objects option was given. This was kept\n> across a bit of refactoring done by 2ace9727be (\"bisect: move common\n> bisect functionality to \"bisect_common\"\", 2009-04-19) and survives\n> to this day.\n>\n> However, \"git bisect\" does not care about tree/blob object\n> reachability at all---it purely works at the commit DAG level and\n> nobody passes (and nobody should pass) \"--objects\" option to the\n> underlying rev-list machinery. Remove the dead code.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Christian, do you recall what we were thinking when we added this\n> mark_edges_uninteresting() call in this program? If you don't,\n> don't worry--this was done more than 8 years ago. I am just\n> being curious and also a bit being cautious in case I am missing\n> something.\n\nI think I just copy pasted the code from cmd_rev_list() in\nbuiltin-rev-list.c and probably didn't realize that revs->tree_objects\nwould always be false.\n\nThanks for spotting this and removing the dead code.\n"},{"id":"313247","messageId":"D9E30A61FAC84D3B90981B388C9699D0@PhilipOakley","threadId":"45281","inReplyTo":"xmqqvarq9vzo.fsf@gitster.mtv.corp.google.com","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-03-03T23:28:39Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> Ever since \"bisect--helper\" was introduced in 1bf072e366\n> (\"bisect--helper: implement \"git bisect--helper\"\", 2009-03-26),\n> after setting up the \"rev-list $bad --not $good_ones\" machinery, the\n> code somehow prepared to mark the trees and blobs at the good boundary\n> as uninteresting, only when --objects option was given. This was kept\n> across a bit of refactoring done by 2ace9727be (\"bisect: move common\n> bisect functionality to \"bisect_common\"\", 2009-04-19) and survives\n> to this day.\n>\n> However, \"git bisect\" does not care about tree/blob object\n> reachability at all---it purely works at the commit DAG level and\n> nobody passes (and nobody should pass) \"--objects\" option to the\n> underlying rev-list machinery. Remove the dead code.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>\n> * Christian, do you recall what we were thinking when we added this\n> mark_edges_uninteresting() call in this program? If you don't,\n> don't worry--this was done more than 8 years ago. I am just\n> being curious and also a bit being cautious in case I am missing\n> something.\n>\n\nBikeshedding: If the given boundary is a tag, it could be tagging a blob or \ntree rather than a commit. Would that be a scenario that reaches this part \nof the code? I thought I read previous comments that there is a case in the \nLinux tree.\n--\nPhilip\n\n> Thanks.\n>\n> bisect.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 30808cadf7..86c5929a23 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)\n> {\n> if (prepare_revision_walk(revs))\n> die(\"revision walk setup failed\");\n> - if (revs->tree_objects)\n> - mark_edges_uninteresting(revs, NULL);\n> }\n>\n> static void exit_if_skipped_commits(struct commit_list *tried,\n> \n\n"},{"id":"313251","messageId":"xmqqd1dx7wbl.fsf@gitster.mtv.corp.google.com","threadId":"45281","inReplyTo":"D9E30A61FAC84D3B90981B388C9699D0@PhilipOakley","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T23:51:58Z","receivedAt":"2017-03-04T00:28:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n> Bikeshedding: If the given boundary is a tag, it could be tagging a\n> blob or tree rather than a commit. Would that be a scenario that\n> reaches this part of the code?\n\nI do not think it is relevant.\n\nBisection is an operation over a \"bisectable\" commit DAG, where\ncommits can be partitioned into \"new\" (aka \"bad\") and \"old\" (aka\n\"good\") camp, all descendants of a \"new\" commit are all \"new\", all\nancestors of an \"old\" commit are all \"old\". Think of the \"new\"-ness\nas a 100% inheritable disease that happens spontaneously and without\ncure. Once you are infected with \"new\"-ness, all your descendants\nare forever \"new\". If you know you are free of the \"new\"-ness, you\nknow that all your ancestors are not with the \"new\"-ness, either.\n\nThe goal of the operation is to find a \"new\" commit whose parents\nare all \"old\".\n\nThe bisectability of the commit DAG is what allows you to say \"this\ncommit is new\" to a commit somewhere in the middle of the history\nand avoid having to test any descendants, as they all inherit the\n\"new\"-ness from it (similarly when you have one commit that is\n\"old\", you do not have to test any ancestor), thereby reducing the\nnumber of test from N (all commits in good..bad range) to log(N).\n\nThere is no room for a tree or a blob to participate in this graph\npartitioning problem. A \"bad\" tree that is \"new\" cannot infect its\nchildren with the \"new\"-ness and a \"good\" tree cannot guarantee the\nlack of \"new\"-ness of its parents, because a tree or a blob does not\nhave parent or child commits.\n"},{"id":"313295","messageId":"5556D465CF3A42C8B117759A170FE3A7@PhilipOakley","threadId":"45281","inReplyTo":"xmqqd1dx7wbl.fsf@gitster.mtv.corp.google.com","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-03-05T15:10:36Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> \"Philip Oakley\" writes:\n>\n>> Bikeshedding: If the given boundary is a tag, it could be tagging a\n>> blob or tree rather than a commit. Would that be a scenario that\n>> reaches this part of the code?\n>\n> I do not think it is relevant.\n>\n> Bisection is an operation over a \"bisectable\" commit DAG, where\n> commits can be partitioned into \"new\" (aka \"bad\") and \"old\" (aka\n> \"good\") camp, all descendants of a \"new\" commit are all \"new\", all\n> ancestors of an \"old\" commit are all \"old\". Think of the \"new\"-ness\n> as a 100% inheritable disease that happens spontaneously and without\n> cure. Once you are infected with \"new\"-ness, all your descendants\n> are forever \"new\". If you know you are free of the \"new\"-ness, you\n> know that all your ancestors are not with the \"new\"-ness, either.\n>\n> The goal of the operation is to find a \"new\" commit whose parents\n> are all \"old\".\n>\n> The bisectability of the commit DAG is what allows you to say \"this\n> commit is new\" to a commit somewhere in the middle of the history\n> and avoid having to test any descendants, as they all inherit the\n> \"new\"-ness from it (similarly when you have one commit that is\n> \"old\", you do not have to test any ancestor), thereby reducing the\n> number of test from N (all commits in good..bad range) to log(N).\n>\n> There is no room for a tree or a blob to participate in this graph\n> partitioning problem. A \"bad\" tree that is \"new\" cannot infect its\n> children with the \"new\"-ness and a \"good\" tree cannot guarantee the\n> lack of \"new\"-ness of its parents, because a tree or a blob does not\n> have parent or child commits.\n>\nThanks.\n\nI was happy with how the bisect actually works. I was more responding the \nthe open question about how that piece of code may have come into existance, \nand the potential thought processes that can lead to such 'mistakes'.\n\nMy line of reasoning was that it is reasonable to pass both commits and \ntags, as revisions(7), to the cli as being the bad and good points in the \ngraph. This could then lead to a expectation that the objects they point to \nshould be suitably marked, which is quite reasonable for commits, and for \nmost tags.\n\nHowever there are tags that point to the commit tree, rather than the commit \nitself, so if that initial rule was followed in a simplistic manner, then \n(falsely) the peeled tree of the tag would be marked as good/bad, which as \nyour patch identifies, would be the wrong thing to do.\n\nThe study of human error is quite interesting....\n\nregards\n\nPhilip\n\n\n"},{"id":"313356","messageId":"xmqqbmtenoji.fsf@junio-linux.mtv.corp.google.com","threadId":"45281","inReplyTo":"5556D465CF3A42C8B117759A170FE3A7@PhilipOakley","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-06T20:22:25Z","receivedAt":"2017-03-06T20:28:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n> The study of human error is quite interesting....\n\nYes ;-)\n"},{"id":"313357","messageId":"xmqqfuiqnokp.fsf@junio-linux.mtv.corp.google.com","threadId":"45281","inReplyTo":"CAP8UFD0tzH+QiNM2BMhe9qcKdD0rP9T=Ry94=EPV+=d+7cdLfQ@mail.gmail.com","subject":"Re: bisect-helper: we do not bisect --objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-06T20:21:42Z","receivedAt":"2017-03-06T20:28:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> I think I just copy pasted the code from cmd_rev_list() in\n> builtin-rev-list.c and probably didn't realize that revs->tree_objects\n> would always be false.\n>\n> Thanks for spotting this and removing the dead code.\n\nThanks for a quick confirmation. Just FYI I was looking at this code\nbecause I was trying to assess how involved it would be to teach the\ncode to do a bisection only on the first-parent chain.\n"}]}