Re: [PATCH] bisect: Store first bad commit as comment in log file

From: Torstein Hegge <hegge@resisty.net>
Date: Apr 15, 2013, 09:53 UTC
Message-ID: <20130415095339.GA28480@pvv.ntnu.no>
In-Reply-To: <20130415.063809.1055555229072260139.chriscool@tuxfamily.org>

On Mon, Apr 15, 2013 at 06:38:09 +0200, Christian Couder wrote:

> I wonder if we should also write something into the bisect log if for
> example the bisection stopped because there are only 'skip'ped commits
> left to test. But maybe this could go into another patch after this
> one.

Yes, that would be useful, but I wasn't able to determine all the cases that would be relevant to log. Only skipped commits left to test is one, but bisect--helper also exits on various problems related to merge base handling. The handling of problems related to inconsistent user input is probably not relevant to log.

I think the successful bisect case is most important to log and the one that requires the least amount of invasive changes.

Show 15 quoted lines

> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index 99efbe8..c58eea7 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -311,7 +311,13 @@ bisect_next() {
> >  	res=$?
> >  
> >  	# Check if we should exit because bisection is finished
> > -	test $res -eq 10 && exit 0
> > +	if test $res -eq 10
> > +	then
> > +		bad_rev=$(git show-ref --hash --verify refs/bisect/bad)
> 
> I had a look to make sure that refs/bisect/bad always refered to the
> first bad commit at this point, and it is true indeed.

According to Documentation/git-bisect.txt, refs/bisect/bad is the proper way to determine the first bad commit at the end of a bisection.

> Maybe you could have used "git rev-parse --verify" instead of "git
> show-ref --hash --verify". It looks simpler to me.

I was wondering why "git grep show-ref *.sh" gave so few users. It looks like rev-parse is more common.

> And maybe, just in case, you could have added: || die "$(gettext "Bad rev: refs/bisect/bad")"

Yes, I should probably have done that.

> Otherwise this patch looks good to me.

Thanks.

Torstein