threads / patch / 39581

patch, 4 partsbisect: add the terms old/new

Subject: [PATCH 4/4] bisect: add the terms old/new

## tl;dr

5 messages between Jun 10, 2015 and Jun 10, 2015. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Antoine Delaite· Jun 10, 2015, 07:11 UTC · lore
Hi,
thanks for the review, 
(sorry if you received this twice) 
Junio C Hamano <gitster@pobox.com> 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.

Show 17 quoted lines
>> * 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· Jun 10, 2015, 08:09 UTC · re: Antoine Delaite · lore

Re: [PATCH 4/4] bisect: add the terms old/new

Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
Show 15 quoted lines
> Hi,
>
> thanks for the review, 
>
> (sorry if you received this twice) 
>
> Junio C Hamano <gitster@pobox.com> 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· Jun 10, 2015, 15:24 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4] bisect: add the terms old/new

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 21 quoted lines
> "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· Jun 10, 2015, 15:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] bisect: add the terms old/new

On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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· Jun 10, 2015, 16:08 UTC · re: Christian Couder · lore

Re: [PATCH 4/4] bisect: add the terms old/new

Christian Couder <christian.couder@gmail.com> writes:
Show 14 quoted lines
> On Wed, Jun 10, 2015 at 5:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> 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.

← back to recent threads