Re: [PATCH 4/4] bisect: add the terms old/new
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] bisect: add the terms old/new
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] bisect: add the terms old/new
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] bisect: add the terms old/new
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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] bisect: add the terms old/new
Antoine Delaite writes:
> When not looking for a regression during a bisect but for a fix or a
> change in another given property, it can be confusing to use 'good'
> and 'bad'.
>
> This patch introduce `git bisect new` and `git bisect old` as an
> alternative to 'bad' and good': the commits which have a certain
> property must be marked as `new` and the ones which do not as `old`.
>
> The output will be the first commit after the change in the property.
> During a new/old bisect session you cannot use bad/good commands and
> vice-versa.
>
> `git bisect replay` works fine for old/new bisect sessions.
>
> Some commands are still not available for old/new:
>
> * git bisect start [ [...]] is not possible: the
>commits will be treated as bad and good.
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
(where the default mode is "git bisect terms bad good") to set up
the "terms", and then after that
$ git bisect start $new $old1 $old2...
would be treated as you would naturally expect, no?
> * 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.
> @@ -379,6 +408,21 @@ In this case, when 'git bisect run' finishes, bisect/bad
> will refer to a commit
> has at least one parent whose reachable graph is fully traversable in the
> sense
> required by 'git pack objects'.
>
> +* Look for a fix instead of a regression in the code
> ++
> +
> +$ git bisect start
> +$ git bisect new HEAD# current commit is marked as new
> +$ git bisect old HEAD~10 # the tenth commit from now is marked as old
> +
> ++
> +If the last commit has a given property, and we are looking for the commit
> +which introduced this property.
Hmph, that is not a complete sentence and I cannot understand what
it is trying to say.
> +For each commit the bisection guide us to we
s/guide us to we/guides us to, we/;
> +will test if the property is present, if it is we will mark the commit as new
> +with 'git bisect new', otherwise we will mark it as old.
It would be easier to read as separate sentences, i.e.
s/is present, if it is/is present. If it is/;
> diff --git a/bisect.c b/bisect.c
> index 3b7df85..511b905 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -409,7 +409,8 @@ static int register_ref(const char *refname, const
> unsigned char *sha1,
> if (!strcmp(refname, name_bad)) {
> current_bad_oid = xmalloc(sizeof(*current_bad_oid));
> hashcpy(current_bad_oid->hash, sha1);
> - } else if (starts_with(refname, "good-")) {
> + } else if (starts_with(refname, "good-") ||
> +starts_with(refname, "old-")) {
> sha1_array_append(&good_revs, sha1);
> } else if (starts_with(refname, "skip-")) {
> sha1_array_append(&skipped_revs, sha1);
> @@ -741,6 +742,12 @@ static void handle_bad_merge_base(void)
> "between %s and [%s].\n",
> bad_hex, bad_hex, good_hex);
> }
> + else if (!strcmp(name_bad, "new")) {
> + fprintf(stderr, "The merge base %s is new.\n"
> + "The property has changed "
> + "between %s and [%s].\n",
> + bad_hex, bad_hex, good_hex);
> + }
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.
> @@ -439,7 +441,7 @@ bisect_replay () {
> start)
> cmd="bisect_start $rev"
> eval "$cmd" ;;
> - good|bad|skip)
> + good|bad|skip|old|new)
Not NAME_GOOD / NAME_BAD?
To support replay, you may want to consider the "bisect terms"
approach and when the bisection is using non-standard terms record
that as the first entry to the bisect log.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
