[PATCH] Bisect: add checks at the beginning of "git bisect run".

[PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

We may be able to "run" with only one good revision given
and then verify that the result of the first run is bad.
And perhaps also the other way around.

But for now let's check that we have at least one bad and
one good revision before we start to run.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-bisect.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index fda1712..57d6754 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -223,6 +223,14 @@ bisect_replay () {
 }
 
 bisect_run () {
+    # Check that we have everything to run correctly.
+    test -d "$GIT_DIR/refs/bisect" || {
+	echo >&2 'You need to start by "git bisect start".'
+	echo >&2 'And then by "git bisect bad" and "git bisect good".'
+	exit 1
+    }
+    bisect_next_check fail
+
     while true
     do
       echo "running $@"
-- 
1.5.1.rc2.2.gcc08

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> diff --git a/git-bisect.sh b/git-bisect.sh
> index fda1712..57d6754 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -223,6 +223,14 @@ bisect_replay () {
>  }
>  
>  bisect_run () {
> +    # Check that we have everything to run correctly.
> +    test -d "$GIT_DIR/refs/bisect" || {
> +	echo >&2 'You need to start by "git bisect start".'
> +	echo >&2 'And then by "git bisect bad" and "git bisect good".'
> +	exit 1
> +    }
> +    bisect_next_check fail
> +

I think you shouldn't need the first "test -d blah"; doesn't
"bisect_next_check fail" already give that message?

>      while true
>      do
>        echo "running $@"
> -- 
> 1.5.1.rc2.2.gcc08

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> We may be able to "run" with only one good revision given
> and then verify that the result of the first run is bad.
> And perhaps also the other way around.

Bisect fundamentally needs one bad commit (nearer to the tip,
relative to the good commits if you have any) to work, but there
is no technical reason to require a good commit.

If the commit at the tip of 'master' branch is bad, it is
entirely possible to start bisection with:

	$ git rev-list --bisect master

It will find a midpoint between the root commit and the HEAD.

However, for a project with long history, if you immediately
start bisecting once you give a bad commit, it would translate
to this rather awkward command sequence, especially when you
already know one good and bad commits:

	$ git bisect start
	$ git bisect bad master
        ... computes the midpoint in the history, which may be
        ... way in the past, and checkout that ancient commit
        ... to test.  however, because you knew a much more
        ... recent commit that is good, you do not bother to
	... test such an ancient one.
	$ git bisect good $known_good_commit
        ... computes the midpoint between $known_good_commit and
        ... master, and checks it out.

For a toy-sized project such as git.git itself, checking out an
ancient revision once, only to immediately check out a more
recent revision, is not much of an overhead, but as your project
grows into real size, such an unnecessary checkout would waste
time (because you would need to update thousands of paths, only
to immediately discard) and buffer cache (because the more
recent, relevant revision would have set of paths much closer to
what you originally had than the initial, wasteful checkout of
the ancient commit).

So our requirement to have at least one good commit is not a
fundamental one, but only a practical one.

We could give an --immediate (or --no-good) option to 'git
bisect bad' to start bisecting as soon as you give a single bad
commit.  It might turn out that the commits you test are bad all
the way down to the root commit, though ;-).

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Le mardi 27 mars 2007 07:28, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> >
> >  bisect_run () {
> > +    # Check that we have everything to run correctly.
> > +    test -d "$GIT_DIR/refs/bisect" || {
> > +	echo >&2 'You need to start by "git bisect start".'
> > +	echo >&2 'And then by "git bisect bad" and "git bisect good".'
> > +	exit 1
> > +    }
> > +    bisect_next_check fail
> > +
>
> I think you shouldn't need the first "test -d blah"; doesn't
> "bisect_next_check fail" already give that message?

Without the first "test -d blah", if "bisect start" was not already used, 
then the user will only see :

'You need to give me at least one good and one bad revisions.'

And then when using "git bisect good" or "git bisect bad" to give good or 
bad revision:

'You need to start by "git bisect start"
Do you want me to do it for you [Y/n]?'

So I thought that it would be better to tell the user upfront everything 
that should be done.

But it's up to you.

Thanks,
Christian.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Without the first "test -d blah", if "bisect start" was not already used, 
> then the user will only see :
>
> 'You need to give me at least one good and one bad revisions.'
>
> And then when using "git bisect good" or "git bisect bad" to give good or 
> bad revision:
>
> 'You need to start by "git bisect start"
> Do you want me to do it for you [Y/n]?'

Doesn't that suggest the existing messages from "git bisect good/bad"
can use the same improvement you added only to "bisect run"?

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Le mardi 27 mars 2007 07:46, Junio C Hamano a écrit :

[...]

> So our requirement to have at least one good commit is not a
> fundamental one, but only a practical one.
>
> We could give an --immediate (or --no-good) option to 'git
> bisect bad' to start bisecting as soon as you give a single bad
> commit.  It might turn out that the commits you test are bad all
> the way down to the root commit, though ;-).

Yes, I plan to add some options to git bisect subcommands.

For example "git bisect run" could accept the following options:

--not
mark current revision as bad instead of good and as good instead of bad

--strict
all exit code except 0 and 1 abort the bisect run process

--good <rev1>
--bad <rev2>
use rev1 as good and rev2 as bad

--check or --test
run the script once and then do nothing if the result is good

Are there other options that people want ?

Thanks,
Christian.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> For example "git bisect run" could accept the following options:
>
> --not
> mark current revision as bad instead of good and as good instead of bad

Do you mean this is a useful option when the "run-script"
reports failure with 0 exit and success with non-zero exit?  In
other words, exit code has reversed meanings from the usual?

> --strict
> all exit code except 0 and 1 abort the bisect run process

This I can understand...

> --good <rev1>
> --bad <rev2>
> use rev1 as good and rev2 as bad

I am not sure what you mean by these two.

> --check or --test
> run the script once and then do nothing if the result is good

How would you use this?  Presumably this makes the command keep
running while the commit is bad, until you hit a good commit and
then stop.

I wonder why this is useful.

Stopping at a commit that happens to be good, without narrowing
the range all the way down is sometimes useful, as you can
usually guess which one of the remaining commits are likely to
be involved with the problem you are seeing, when the remaining
set is sufficiently narrow.  But how narrow the remaining set is
does not have much to do with your finding a good commit for the
first time.  If you start from a single bad commit at the tip of
10-year old project, you would probably try 5-year old commit,
which may well be good (or maybe it is too old to be relevant)
and then this option would make the cycle stop, and you have
5-year worth of history still left to be bisected.  I am not
sure what the assumed workflow would be after that point.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Le mercredi 28 mars 2007 07:46, Junio C Hamano a écrit :
> >
> > --not
> > mark current revision as bad instead of good and as good instead of bad
>
> Do you mean this is a useful option when the "run-script"
> reports failure with 0 exit and success with non-zero exit?  In
> other words, exit code has reversed meanings from the usual?

Yes, for example to find when a string first appeared in a file one could 
use:

git bisect run --not grep string my_file

instead of something like

git bisect run ! grep string my_file

or

git bisect run 'grep string my_file ; test $? -ne 0'

> > --good <rev1>
> > --bad <rev2>
> > use rev1 as good and rev2 as bad
>
> I am not sure what you mean by these two.

For example one could write:

git bisect run --good rev1 --bad rev2 my_script

instead of

git bisect start
git bisect good rev1
git bisect bad rev2
git bisect run my_script

> > --check or --test
> > run the script once and then do nothing if the result is good
>
> How would you use this? 

For example if you know that the last nightly build 
tagged "nightly_2007_03_27" was ok, you could use:

git bisect start
git bisect good nightly_2007_03_27
git bisect run --check make > /dev/null || {
	# extract commit and author email address from "$GIT_DIR/BISECT_RUN"
	# and send flame to author who broke the build with the commit
}

to automatically check that current source code builds ok.

Or you could use it in some test suites to automatically find the first bad 
commit (and be able to flame the author :-) in case one test fails.

Christian.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> git bisect run --not grep string my_file
>
> instead of something like
>
> git bisect run ! grep string my_file

How's the former more useful than the latter was what I was
wondering but I do not care too deeply.

> For example one could write:
>
> git bisect run --good rev1 --bad rev2 my_script
>
> instead of
>
> git bisect start
> git bisect good rev1
> git bisect bad rev2
> git bisect run my_script

Likewise.

>> > --check or --test
>> > run the script once and then do nothing if the result is good
>>
>> How would you use this? 
>
> For example if you know that the last nightly build 
> tagged "nightly_2007_03_27" was ok, you could use:
>
> git bisect start
> git bisect good nightly_2007_03_27
> git bisect run --check make > /dev/null || {
> 	# extract commit and author email address from "$GIT_DIR/BISECT_RUN"
> 	# and send flame to author who broke the build with the commit
> }
>
> to automatically check that current source code builds ok.

As I said, bisect fundamentally needs one bad commit to start
bisecting, so after you give only one good commit, it would not
move to bisect branch nor suggest which commit to test.  I
assume you intend to run the test on the tip of the current
branch here, but then your "git bisect run --check ./run-script"
is equivalent to running ./run-script and checking the exit
value.

In other words, the above would be equilvalent to

  git bisect start
  git bisect good nightly_2007_03_27
  make >/dev/null || {
	  git bisect bad
          git bisect run make >/dev/null
          # extract commit and author email address from "$GIT_DIR/BISECT_RUN"
          # and send flame to author who broke the build with the commit
  }

I happen to find the latter easier to read as a shell script.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Le mercredi 28 mars 2007 09:57, Junio C Hamano a écrit :
>
> In other words, the above would be equilvalent to
>
>   git bisect start
>   git bisect good nightly_2007_03_27
>   make >/dev/null || {
> 	  git bisect bad
>           git bisect run make >/dev/null
>           # extract commit and author email address from
> "$GIT_DIR/BISECT_RUN" # and send flame to author who broke the build with
> the commit }

Yes, it would be equivalent but a little shorter.

So while we are at it, what about a new subcommand "git bisect test" that 
could be used like this:

git bisect test good_rev my_script

equivalent to the following :

my_script || {
	git bisect start &&
	git bisect bad &&
	git bisect good good_rev &&
	git bisect run my_script
}

that could be used like this:

git bisect test good_rev my_script || {
	echo >&2 "my_script failed but 'git bisect test' failed too"
	exit 1
}
test -f "$GIT_DIR/BISECT_RUN" && {
	# extract commit and author email address from
	# "$GIT_DIR/BISECT_RUN" and send flames
	git bisect reset
}

Thanks,
Christian.

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> So while we are at it, what about a new subcommand "git bisect test" that 
> could be used like this:

You are using it as a building block for nightly script, so
there are already certain logic around the call to "git bisect"
in your script.  I do not think we would necessarily want more
subcommands to "git bisect", unless the new subcommands truly is
generally applicable.

Unlike "run", "test" expects a workflow where there always is
one recent good_rev and there might be at most one single
breakage introduced since the last nightly build.  For that
particular use case, it might be handier to be able to write a
single line:

> git bisect test good_rev my_script

compared to:

> my_script || {
> 	git bisect start &&
> 	git bisect bad &&
> 	git bisect good good_rev &&
> 	git bisect run my_script
> }

but it is not useful for use case other than that.  Maybe we
already know more than one good commits that are on the side
branch to limit the bisect space in project specific way.

The thing is, the more you add policy to a building block, the
less generally useful the building block becomes.  The reason I
took "git bisect run" is because for the simplest use case, it
can be used without writing a specialized "run script" (you can
give "make test" to it, for example).  And more importantly, in
the case of "run", there is not much policy involved.  It is a
good command to have in a building block because what it does is
purely to automate what the user would perform mechanically by
hand anyway.  One thing I would want is to keep the bisect
subcommands to the minimum, so that people can easily use it as
a building block in their automation, without having to hunt
through many workflow-specific subcommands that do not suit
their particular needs.  Catering to their particular needs are
better handled in their scripts, including your "I have one
known good commit, I do not know if the tip is good, and I want
to dig down from the tip only when the tip is bad" case.

If you want to add value to bisect, here are two I can think of,
one almost trivial, and the other a bit harder.

(1) One bad commit is fundamentally needed for bisect to run,
    and if we beforehand know more good commits, we can narrow
    the bisect space down without doing the whole tree checkout
    every time we give good commits.  I think it may be a good
    idea to have:

	git bisect start [$bad [$good1 $good2...]] [-- <paths>...]

    as a short-hand for this command sequence:

	git bisect start
        git bisect bad $bad
        git bisect good $good1 $good2...

    That would be a good script-shorterner, without limiting it to
    any specific use scenario.

(2) There is no technical reason to have a good commit for
    bisect to run, other than you _often_ do not want the first
    mid-point checkout before you give good ones to it.  But
    sometimes, you may not know even if something _ever_ worked,
    IOW, even the root commit might not be good.  In such a
    case, being able to bisect having only one bad commit and no
    good commit would be handy.  For this, bisect_next_check
    needs to be tweaked.  Probably a transcript for such a
    feature would go like this:

	$ git bisect start
        $ git bisect bad HEAD
        $ git bisect next ; echo $?
        You need to give me at least one good and one bad revisions,
	with "git bisect good" and "git bisect bad".
	1
	$ git bisect next --without-good
	Bisecting: 4321 revisions left to test after this
	[deadcafedeadcafedeadcafedeadcafedeadcafe] an ancient commit
	$ git bisect bad
	Bisecting: 2152 revisions left to test after this
	[edeadcafedeadcafedeadcafedeadcafedeadcaf] a more ancient commit

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Le jeudi 29 mars 2007 08:06, Junio C Hamano a écrit :

[...]

> The thing is, the more you add policy to a building block, the
> less generally useful the building block becomes.  The reason I
> took "git bisect run" is because for the simplest use case, it
> can be used without writing a specialized "run script" (you can
> give "make test" to it, for example).  And more importantly, in
> the case of "run", there is not much policy involved.  It is a
> good command to have in a building block because what it does is
> purely to automate what the user would perform mechanically by
> hand anyway.  One thing I would want is to keep the bisect
> subcommands to the minimum, so that people can easily use it as
> a building block in their automation, without having to hunt
> through many workflow-specific subcommands that do not suit
> their particular needs. 

I understand this.

> Catering to their particular needs are 
> better handled in their scripts, including your "I have one
> known good commit, I do not know if the tip is good, and I want
> to dig down from the tip only when the tip is bad" case.

But I think this is not a specific need. Many people are doing nightly 
builds and it's a good practice, so we should encourage them by making it 
as easy as possible.

Perhaps a new git subcommand instead of a git bisect subcommand.

For a nightly build you want to do something like:

my_build_script || {
	git bisect start &&
	git bisect bad &&
	git bisect good good_rev &&
	git bisect run my_script
}

> If you want to add value to bisect, here are two I can think of,
> one almost trivial, and the other a bit harder.
>
> (1) One bad commit is fundamentally needed for bisect to run,
>     and if we beforehand know more good commits, we can narrow
>     the bisect space down without doing the whole tree checkout
>     every time we give good commits.  I think it may be a good
>     idea to have:
>
> 	git bisect start [$bad [$good1 $good2...]] [-- <paths>...]
>
>     as a short-hand for this command sequence:
>
> 	git bisect start
>         git bisect bad $bad
>         git bisect good $good1 $good2...
>
>     That would be a good script-shorterner, without limiting it to
>     any specific use scenario.
>
> (2) There is no technical reason to have a good commit for
>     bisect to run, other than you _often_ do not want the first
>     mid-point checkout before you give good ones to it.  But
>     sometimes, you may not know even if something _ever_ worked,
>     IOW, even the root commit might not be good.  In such a
>     case, being able to bisect having only one bad commit and no
>     good commit would be handy.  For this, bisect_next_check
>     needs to be tweaked.  Probably a transcript for such a
>     feature would go like this:
>
> 	$ git bisect start
>         $ git bisect bad HEAD
>         $ git bisect next ; echo $?
>         You need to give me at least one good and one bad revisions,
> 	with "git bisect good" and "git bisect bad".
> 	1
> 	$ git bisect next --without-good
> 	Bisecting: 4321 revisions left to test after this
> 	[deadcafedeadcafedeadcafedeadcafedeadcafe] an ancient commit
> 	$ git bisect bad
> 	Bisecting: 2152 revisions left to test after this
> 	[edeadcafedeadcafedeadcafedeadcafedeadcaf] a more ancient commit

Re: [PATCH] Bisect: add checks at the beginning of "git bisect run".

[Christian Couder]

Ooops again! Sorry, this message was sent before it was finished.
Please ignore it. I will resend a good one latter.

Thanks,
Christian.


Le jeudi 5 avril 2007 10:01, Christian Couder a écrit :
> Le jeudi 29 mars 2007 08:06, Junio C Hamano a écrit :
>
> [...]
>
> > The thing is, the more you add policy to a building block, the
> > less generally useful the building block becomes.  The reason I
> > took "git bisect run" is because for the simplest use case, it
> > can be used without writing a specialized "run script" (you can
> > give "make test" to it, for example).  And more importantly, in
> > the case of "run", there is not much policy involved.  It is a
> > good command to have in a building block because what it does is
> > purely to automate what the user would perform mechanically by
> > hand anyway.  One thing I would want is to keep the bisect
> > subcommands to the minimum, so that people can easily use it as
> > a building block in their automation, without having to hunt
> > through many workflow-specific subcommands that do not suit
> > their particular needs.
>
> I understand this.
>
> > Catering to their particular needs are
> > better handled in their scripts, including your "I have one
> > known good commit, I do not know if the tip is good, and I want
> > to dig down from the tip only when the tip is bad" case.
>
> But I think this is not a specific need. Many people are doing nightly
> builds and it's a good practice, so we should encourage them by making it
> as easy as possible.
>
> Perhaps a new git subcommand instead of a git bisect subcommand.
>
> For a nightly build you want to do something like:
>
> my_build_script || {
> 	git bisect start &&
> 	git bisect bad &&
> 	git bisect good good_rev &&
> 	git bisect run my_script
> }

[...]