# [PATCH 4/4] bisect: add the terms old/new 5 messages from 2015-06-10 to 2015-06-10. Participants: Antoine Delaite, Matthieu Moy, Junio C Hamano, Christian Couder. Thread: https://gitlist.dev/t/39581 ## Antoine Delaite, 2015-06-10 07:11 Subject: [PATCH 4/4] bisect: add the terms old/new Message-ID: <1839018688.331427.1433920317370.JavaMail.zimbra@ensimag.grenoble-inp.fr> URL: https://gitlist.dev/e/1839018688.331427.1433920317370.JavaMail.zimbra%40ensimag.grenoble-inp.fr In-Reply-To: <78277223.323387.1433874176840.JavaMail.zimbra@ensimag.grenoble-inp.fr> ``` Hi, thanks for the review, (sorry if you received this twice) Junio C Hamano writes: >Just throwing a suggestion. You could perhaps add a new verb to be >used before starting to do anything, e.g. > > $ git bisect terms new old Yes it would be nice and should not be hard to implement. But it was not the idea of how the code was made by our elders. For now we just rebased, corrected and finishing to implement functionalities. >> * git rev-list --bisect does not treat the revs/bisect/new and >> revs/bisect/old-SHA1 files. > >That shouldn't be hard to add, I would imagine, either by > > git rev-list --bisect=new,old > >or teaching "git rev-list --bisect" to read the "terms" itself, as a >follow-up patch. > >> * git bisect visualize seem to work partially: the tags are >> displayed correctly but the tree is not limited to the bisect >> section. > >This is directly related to the lack of "git rev-list --bisect" >support, I would imagine. If you make the command read "terms", I >suspect that it would solve itself. It will be corrected in the next patch. >After reading the previous patches in the series, I expected that >this new code would know to read "terms" and does not limit us only >to "new" and "old". Somewhat disappointing. It is still nice and necessary to have new/old as builtin tags but if we have time we will do that. The others point have been taken in account. ``` ## Matthieu Moy, 2015-06-10 08:09 Subject: Re: [PATCH 4/4] bisect: add the terms old/new Message-ID: URL: https://gitlist.dev/e/vpq7frcast5.fsf%40anie.imag.fr In-Reply-To: <1839018688.331427.1433920317370.JavaMail.zimbra@ensimag.grenoble-inp.fr> ``` Antoine Delaite writes: > Hi, > > thanks for the review, > > (sorry if you received this twice) > > Junio C Hamano writes: > >>Just throwing a suggestion. You could perhaps add a new verb to be >>used before starting to do anything, e.g. >> >> $ git bisect terms new old > > Yes it would be nice and should not be hard to implement. But it was not > the idea of how the code was made by our elders. "Somebody else did it like that" is not a good justification. Especially when the previous code was not merged: the code wasn't finished. But I actually disagree with the fact that it was not the idea. The point of having the terms in BISECT_TERMS was precisely to be generic enough. Had the goal been just to distinguish good/bad and old/new, we would have needed only one bit of information, and encoding it with the existance/non-existance of a file would have been sufficient (as you tried to do in addition to BISECT_TERMS). > For now we just rebased, corrected and finishing to implement > functionalities. functionalities is one thing, but the code should be maintainable to be merged in git.git. Git would not be where it is if Junio was merging patches based on "it works, we'll see if the code is good enough later" kinds of judgments ;-). Moving from "one hardcoded pair of terms" to "two hardcoded pairs of terms" is a nice feature, but hardly a step in the right direction wrt maintainability. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2015-06-10 15:24 Subject: Re: [PATCH 4/4] bisect: add the terms old/new Message-ID: URL: https://gitlist.dev/e/xmqqwpzbmvr3.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > "Somebody else did it like that" is not a good justification. Especially > when the previous code was not merged: the code wasn't finished. > > But I actually disagree with the fact that it was not the idea. The > point of having the terms in BISECT_TERMS was precisely to be generic > enough. Had the goal been just to distinguish good/bad and old/new, we > would have needed only one bit of information, and encoding it with the > existance/non-existance of a file would have been sufficient (as you > tried to do in addition to BISECT_TERMS). > >> For now we just rebased, corrected and finishing to implement >> functionalities. > > functionalities is one thing, but the code should be maintainable to be > merged in git.git. Git would not be where it is if Junio was merging > patches based on "it works, we'll see if the code is good enough later" > kinds of judgments ;-). > > Moving from "one hardcoded pair of terms" to "two hardcoded pairs of > terms" is a nice feature, but hardly a step in the right direction wrt > maintainability. Nicely put. From that point of view, the variable names and the underlying machinery in general should call these two "new" vs "old". I.e. name_new=bad name_old=good would be the default, not name_bad=bad name_good=good. ``` ## Christian Couder, 2015-06-10 15:47 Subject: Re: [PATCH 4/4] bisect: add the terms old/new Message-ID: URL: https://gitlist.dev/e/CAP8UFD0-R1UqSDbEeTKq_gX%2BuFw2egZetgziqaUiRqdivHvhPA%40mail.gmail.com In-Reply-To: ``` On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano wrote: > Matthieu Moy writes: > >> "Somebody else did it like that" is not a good justification. Especially >> when the previous code was not merged: the code wasn't finished. >> >> But I actually disagree with the fact that it was not the idea. The >> point of having the terms in BISECT_TERMS was precisely to be generic >> enough. Had the goal been just to distinguish good/bad and old/new, we >> would have needed only one bit of information, and encoding it with the >> existance/non-existance of a file would have been sufficient (as you >> tried to do in addition to BISECT_TERMS). >> >>> For now we just rebased, corrected and finishing to implement >>> functionalities. >> >> functionalities is one thing, but the code should be maintainable to be >> merged in git.git. Git would not be where it is if Junio was merging >> patches based on "it works, we'll see if the code is good enough later" >> kinds of judgments ;-). >> >> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of >> terms" is a nice feature, but hardly a step in the right direction wrt >> maintainability. > > Nicely put. From that point of view, the variable names and the > underlying machinery in general should call these two "new" vs > "old". I.e. name_new=bad name_old=good would be the default, not > name_bad=bad name_good=good. I don't think this would improve maintainability, at least not for me. The doc currently uses "good/bad" everywhere. For example the description is: This command uses git rev-list --bisect to help drive the binary search process to find which change introduced a bug, given an old "good" commit object name and a later "bad" commit object name. And this is logical if the default is "good/bad". If we use name_new and name_old in the code, we make it harder for us to see if the doc matches the code. And unless we rewrite a lot of them, the tests too will mostly be still using "good/bad" so it will make it harder to maintain them too. ``` ## Junio C Hamano, 2015-06-10 16:08 Subject: Re: [PATCH 4/4] bisect: add the terms old/new Message-ID: URL: https://gitlist.dev/e/xmqqsi9zmtqm.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Christian Couder writes: > On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano wrote: >> Matthieu Moy writes: >>> >>> Moving from "one hardcoded pair of terms" to "two hardcoded pairs of >>> terms" is a nice feature, but hardly a step in the right direction wrt >>> maintainability. >> >> Nicely put. From that point of view, the variable names and the >> underlying machinery in general should call these two "new" vs >> "old". I.e. name_new=bad name_old=good would be the default, not >> name_bad=bad name_good=good. > > I don't think this would improve maintainability, at least not for me. > The doc currently uses "good/bad" everywhere. You are conflating the internal implementation and the end-user facing interface, I think. The topic under discussion is about updating the internal implementation more generic and make it capable of handling both the traditional and the default 'find transition from good to bad' and any other kinds that can be expressed by 'find transition from $old to $new' where the values of $old and $new can be specified by the end user. And then we keep old=good new=bad as the default. And the best time to update the implementation to express its operation in terms of 'find transition from $old to $new' is when such a feature is introduced, in other words, inside this topic. If you still are thinking in terms of 'find transition from $good to $bad', then I can understand that you would disagree with the above, but I think you are on the same page as Matthieu and me that we are updating the system to use "'find transition from $old to $new' where the values of $old and $new can be specified by the end user" as the new paradigm. And it is perfectly fine for the documentation to talk about the feature using the default pair of words. Hopefully this clarifies. ```