Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()
Christian Couder chrisc...@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?
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)?
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
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.
--
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 1/2] bisect: parse revs before passing them to check_expected_revs()
From: Junio C Hamano gits...@pobox.com
Christian Couder chrisc...@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.
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.
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.
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.
--
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
[PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()
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 chrisc...@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(-)
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
--
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
