{"thread":{"id":"39581","subject":"[PATCH 4/4] bisect: add the terms old/new","startedAt":"2015-06-10T07:11:57Z","lastAt":"2015-06-10T16:08:17Z","messageCount":5,"participants":["Antoine Delaite","Matthieu Moy","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"263437","messageId":"1839018688.331427.1433920317370.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39581","inReplyTo":"78277223.323387.1433874176840.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"[PATCH 4/4] bisect: add the terms old/new","fromName":"Antoine Delaite","fromEmail":"antoine.delaite@ensimag.grenoble-inp.fr","sentAt":"2015-06-10T07:11:57Z","receivedAt":"2015-06-10T07:11:57Z","isPatch":true,"sender":{"key":"antoine.delaite@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12510393?v=4"},"body":"Hi,\n\nthanks for the review, \n\n(sorry if you received this twice) \n\nJunio C Hamano writes: \n\n>Just throwing a suggestion. You could perhaps add a new verb to be \n>used before starting to do anything, e.g. \n> \n> $ git bisect terms new old \n\nYes it would be nice and should not be hard to implement. But it was not \nthe idea of how the code was made by our elders. For now we just rebased, \ncorrected and finishing to implement functionalities. \n\n>> * git rev-list --bisect does not treat the revs/bisect/new and \n>> revs/bisect/old-SHA1 files. \n> \n>That shouldn't be hard to add, I would imagine, either by \n> \n> git rev-list --bisect=new,old\n> \n>or teaching \"git rev-list --bisect\" to read the \"terms\" itself, as a \n>follow-up patch.\n> \n>> * git bisect visualize seem to work partially: the tags are \n>> displayed correctly but the tree is not limited to the bisect \n>> section. \n> \n>This is directly related to the lack of \"git rev-list --bisect\" \n>support, I would imagine. If you make the command read \"terms\", I \n>suspect that it would solve itself.\n\nIt will be corrected in the next patch.\n\n>After reading the previous patches in the series, I expected that \n>this new code would know to read \"terms\" and does not limit us only \n>to \"new\" and \"old\". Somewhat disappointing.\n\nIt is still nice and necessary to have new/old as builtin tags but \nif we have time we will do that.\n\nThe others point have been taken in account.\n"},{"id":"263450","messageId":"vpq7frcast5.fsf@anie.imag.fr","threadId":"39581","inReplyTo":"1839018688.331427.1433920317370.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 4/4] bisect: add the terms old/new","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T08:09:10Z","receivedAt":"2015-06-10T08:09:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Antoine Delaite writes:\n\n> Hi,\n>\n> thanks for the review, \n>\n> (sorry if you received this twice) \n>\n> Junio C Hamano writes: \n>\n>>Just throwing a suggestion. You could perhaps add a new verb to be \n>>used before starting to do anything, e.g. \n>> \n>> $ git bisect terms new old \n>\n> Yes it would be nice and should not be hard to implement. But it was not \n> the idea of how the code was made by our elders.\n\n\"Somebody else did it like that\" is not a good justification. Especially\nwhen the previous code was not merged: the code wasn't finished.\n\nBut I actually disagree with the fact that it was not the idea. The\npoint of having the terms in BISECT_TERMS was precisely to be generic\nenough. Had the goal been just to distinguish good/bad and old/new, we\nwould have needed only one bit of information, and encoding it with the\nexistance/non-existance of a file would have been sufficient (as you\ntried to do in addition to BISECT_TERMS).\n\n> For now we just rebased, corrected and finishing to implement\n> functionalities.\n\nfunctionalities is one thing, but the code should be maintainable to be\nmerged in git.git. Git would not be where it is if Junio was merging\npatches based on \"it works, we'll see if the code is good enough later\"\nkinds of judgments ;-).\n\nMoving from \"one hardcoded pair of terms\" to \"two hardcoded pairs of\nterms\" is a nice feature, but hardly a step in the right direction wrt\nmaintainability.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"263481","messageId":"xmqqwpzbmvr3.fsf@gitster.dls.corp.google.com","threadId":"39581","inReplyTo":"vpq7frcast5.fsf@anie.imag.fr","subject":"Re: [PATCH 4/4] bisect: add the terms old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-10T15:24:48Z","receivedAt":"2015-06-10T15:24:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> \"Somebody else did it like that\" is not a good justification. Especially\n> when the previous code was not merged: the code wasn't finished.\n>\n> But I actually disagree with the fact that it was not the idea. The\n> point of having the terms in BISECT_TERMS was precisely to be generic\n> enough. Had the goal been just to distinguish good/bad and old/new, we\n> would have needed only one bit of information, and encoding it with the\n> existance/non-existance of a file would have been sufficient (as you\n> tried to do in addition to BISECT_TERMS).\n>\n>> For now we just rebased, corrected and finishing to implement\n>> functionalities.\n>\n> functionalities is one thing, but the code should be maintainable to be\n> merged in git.git. Git would not be where it is if Junio was merging\n> patches based on \"it works, we'll see if the code is good enough later\"\n> kinds of judgments ;-).\n>\n> Moving from \"one hardcoded pair of terms\" to \"two hardcoded pairs of\n> terms\" is a nice feature, but hardly a step in the right direction wrt\n> maintainability.\n\nNicely put. From that point of view, the variable names and the\nunderlying machinery in general should call these two \"new\" vs\n\"old\". I.e. name_new=bad name_old=good would be the default, not\nname_bad=bad name_good=good.\n"},{"id":"263486","messageId":"CAP8UFD0-R1UqSDbEeTKq_gX+uFw2egZetgziqaUiRqdivHvhPA@mail.gmail.com","threadId":"39581","inReplyTo":"xmqqwpzbmvr3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/4] bisect: add the terms old/new","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-06-10T15:47:13Z","receivedAt":"2015-06-10T15:47:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n>\n>> \"Somebody else did it like that\" is not a good justification. Especially\n>> when the previous code was not merged: the code wasn't finished.\n>>\n>> But I actually disagree with the fact that it was not the idea. The\n>> point of having the terms in BISECT_TERMS was precisely to be generic\n>> enough. Had the goal been just to distinguish good/bad and old/new, we\n>> would have needed only one bit of information, and encoding it with the\n>> existance/non-existance of a file would have been sufficient (as you\n>> tried to do in addition to BISECT_TERMS).\n>>\n>>> For now we just rebased, corrected and finishing to implement\n>>> functionalities.\n>>\n>> functionalities is one thing, but the code should be maintainable to be\n>> merged in git.git. Git would not be where it is if Junio was merging\n>> patches based on \"it works, we'll see if the code is good enough later\"\n>> kinds of judgments ;-).\n>>\n>> Moving from \"one hardcoded pair of terms\" to \"two hardcoded pairs of\n>> terms\" is a nice feature, but hardly a step in the right direction wrt\n>> maintainability.\n>\n> Nicely put. From that point of view, the variable names and the\n> underlying machinery in general should call these two \"new\" vs\n> \"old\". I.e. name_new=bad name_old=good would be the default, not\n> name_bad=bad name_good=good.\n\nI don't think this would improve maintainability, at least not for me.\nThe doc currently uses \"good/bad\" everywhere.\nFor example the description is:\n\n This command uses git rev-list --bisect to help drive the binary search\n process to find which change introduced a bug, given an old \"good\" commit\n object name and a later \"bad\" commit object name.\n\nAnd this is logical if the default is \"good/bad\". If we use name_new\nand name_old in the code, we make it harder for us to see if the doc\nmatches the code.\n\nAnd unless we rewrite a lot of them, the tests too will mostly be\nstill using \"good/bad\" so it will make it harder to maintain them too.\n"},{"id":"263491","messageId":"xmqqsi9zmtqm.fsf@gitster.dls.corp.google.com","threadId":"39581","inReplyTo":"CAP8UFD0-R1UqSDbEeTKq_gX+uFw2egZetgziqaUiRqdivHvhPA@mail.gmail.com","subject":"Re: [PATCH 4/4] bisect: add the terms old/new","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-10T16:08:17Z","receivedAt":"2015-06-10T16:08:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano wrote:\n>> Matthieu Moy writes:\n>>>\n>>> Moving from \"one hardcoded pair of terms\" to \"two hardcoded pairs of\n>>> terms\" is a nice feature, but hardly a step in the right direction wrt\n>>> maintainability.\n>>\n>> Nicely put. From that point of view, the variable names and the\n>> underlying machinery in general should call these two \"new\" vs\n>> \"old\". I.e. name_new=bad name_old=good would be the default, not\n>> name_bad=bad name_good=good.\n>\n> I don't think this would improve maintainability, at least not for me.\n> The doc currently uses \"good/bad\" everywhere.\n\nYou are conflating the internal implementation and the end-user\nfacing interface, I think.\n\nThe topic under discussion is about updating the internal\nimplementation more generic and make it capable of handling both the\ntraditional and the default 'find transition from good to bad' and\nany other kinds that can be expressed by 'find transition from $old\nto $new' where the values of $old and $new can be specified by the\nend user. And then we keep old=good new=bad as the default.\n\nAnd the best time to update the implementation to express its\noperation in terms of 'find transition from $old to $new' is when\nsuch a feature is introduced, in other words, inside this topic.\n\nIf you still are thinking in terms of 'find transition from $good to\n$bad', then I can understand that you would disagree with the above,\nbut I think you are on the same page as Matthieu and me that we are\nupdating the system to use \"'find transition from $old to $new' where\nthe values of $old and $new can be specified by the end user\" as the\nnew paradigm.\n\nAnd it is perfectly fine for the documentation to talk about the\nfeature using the default pair of words.\n\nHopefully this clarifies.\n"}]}