threads / patch / 38251

patch, 2 partsbisect: parse revs before passing them to check_expected_revs()

Subject: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()

## tl;dr

3 messages between Dec 25, 2014 and Dec 29, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Christian Couder· Dec 25, 2014, 18:25 UTC · lore

When running for example "git bisect bad HEAD" or "git bisect good master", the parameter passed to "git bisect (bad|good)" has to be parsed into a commit hash before checking if it is the expected commit or not.

We could do that in is_expected_rev() or in check_expected_revs(), but it is already done in bisect_state(). Let's just store the hash values that result from this parsing, and then reuse them after all the parsing is done.

This way we can also use a for loop over these values to call bisect_write() on them, instead of using eval.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
I think that it is a better patch than the one I sent
previously to the list as with this one we parse revs
only once. 
Merry Christmas!
 git-bisect.sh | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
Show changes to git-bisect.sh +7 −4
diff --git a/git-bisect.sh b/git-bisect.sh
index 6cda2b5..2fc07ac 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,15 +237,18 @@ bisect_state() {
 		check_expected_revs "$rev" ;;
 	2,bad|*,good|*,skip)
 		shift
-		eval=''
+		hash_list=''
 		for rev in "$@"
 		do
 			sha=$(git rev-parse --verify "$rev^{commit}") ||
 				die "$(eval_gettext "Bad rev input: \$rev")"
-			eval="$eval bisect_write '$state' '$sha'; "
+			hash_list="$hash_list $sha"
 		done
-		eval "$eval"
-		check_expected_revs "$@" ;;
+		for rev in $hash_list
+		do
+			bisect_write "$state" "$rev"
+		done
+		check_expected_revs $hash_list ;;
 	*,bad)
 		die "$(gettext "'git bisect bad' can take only one argument.")" ;;
 	*)

-- 
2.1.2.555.gfbecd99
Junio C Hamano· Dec 29, 2014, 19:33 UTC · re: Christian Couder · lore

Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()

Christian Couder <chriscool@tuxfamily.org> writes:
Show 5 quoted lines
> When running for example "git bisect bad HEAD" or
> "git bisect good master", the parameter passed to
> "git bisect (bad|good)" has to be parsed into a
> commit hash before checking if it is the expected
> commit or not.

Hmm, is that because you wrote commit object name in 40-hex in the EXPECTED_REV and you need to compare with what the user gave you which could be symbolic?

The conversion makes sense, but why is it a bad thing to say
	git bisect bad maint

when 'maint' is not what you checked out in the current bisect run in the first place (perhaps you checked if it is good or bad manually before you started bisecting)?

Show 7 quoted lines
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 6cda2b5..2fc07ac 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -237,15 +237,18 @@ bisect_state() {
>  		check_expected_revs "$rev" ;;
>  	2,bad|*,good|*,skip)

This part accepts arbitrary number of revs when running good and skip, e.g.

	git bisect good maint master next
and it loops
Show 10 quoted lines
>  		shift
> -		eval=''
> +		hash_list=''
>  		for rev in "$@"
> ...
> +		for rev in $hash_list
> +		do
> +			bisect_write "$state" "$rev"
> +		done
> +		check_expected_revs $hash_list ;;

But check_expected_revs loops and leaves the loop early when it finds anything that is not expected.

... goes and looks ...

Hmph, I think the logic in check_expected_revs is not wrong, but this helper function is grossly misnamed. It is not checking and rejecting the user input---it is checking to see if it can bypass check_good_are_ancestors_of_bad() which is expensive, so when it sees any one of the input is not what it checked out, it just disables the "optimization".

OK, will queue.
Thanks.
Christian Couder· Dec 29, 2014, 22:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()

From: Junio C Hamano <gitster@pobox.com>
Show 11 quoted lines
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> When running for example "git bisect bad HEAD" or
>> "git bisect good master", the parameter passed to
>> "git bisect (bad|good)" has to be parsed into a
>> commit hash before checking if it is the expected
>> commit or not.
> 
> Hmm, is that because you wrote commit object name in 40-hex in the
> EXPECTED_REV and you need to compare with what the user gave you
> which could be symbolic?
Yes, that's the reason.
Show 7 quoted lines
> The conversion makes sense, but why is it a bad thing to say
> 
> 	git bisect bad maint
> 
> when 'maint' is not what you checked out in the current bisect run
> in the first place (perhaps you checked if it is good or bad manually
> before you started bisecting)?

It is not a "bad thing" to test another commit than the one that has been checked out and then to say if it is "good" or "bad". But if you do that then it is safer to check if a merge base should be tested.

I can discuss this point further and there are indeed some optimisations we could implement in this area, but I think it is better to try to just fix the bug first.

Show 14 quoted lines
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 6cda2b5..2fc07ac 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -237,15 +237,18 @@ bisect_state() {
>>  		check_expected_revs "$rev" ;;
>>  	2,bad|*,good|*,skip)
> 
> This part accepts arbitrary number of revs when running good and
> skip, e.g.
> 
> 	git bisect good maint master next
> 
> and it loops
Yeah.
Show 22 quoted lines
>>  		shift
>> -		eval=''
>> +		hash_list=''
>>  		for rev in "$@"
>> ...
>> +		for rev in $hash_list
>> +		do
>> +			bisect_write "$state" "$rev"
>> +		done
>> +		check_expected_revs $hash_list ;;
> 
> But check_expected_revs loops and leaves the loop early when it
> finds anything that is not expected.
> 
> ... goes and looks ...
> 
> Hmph, I think the logic in check_expected_revs is not wrong, but
> this helper function is grossly misnamed.  It is not checking and
> rejecting the user input---it is checking to see if it can bypass
> check_good_are_ancestors_of_bad() which is expensive, so when it
> sees any one of the input is not what it checked out, it just
> disables the "optimization".

Yeah, that's the idea. If you have a better name for check_expected_revs(), I can change it in another patch.

And yeah, check_good_are_ancestors_of_bad() is expensive to compute and also expensive because it might mean that more tests have to be performed by the user to be safe.

> OK, will queue.

Thanks, Christian.

← back to recent threads