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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Apr 15, 2013, 04:38 UTC
Message-ID: <20130415.063809.1055555229072260139.chriscool@tuxfamily.org>
In-Reply-To: <20130413152257.GB16040@pvv.ntnu.no>

From: Torstein Hegge <hegge@resisty.net>
Subject: [PATCH] bisect: Store first bad commit as comment in log file
Date: Sat, 13 Apr 2013 17:22:57 +0200

Show 6 quoted lines

> When bisect successfully finds a single revision, the first bad commit
> should be shown to human readers of 'git bisect log'.
> 
> This resolves the apparent disconnect between the bisection result and
> the log when a bug reporter says "I know that the first bad commit is
> $rev, as you can see from $(git bisect log)".

I agree that it's a good idea to do that.

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.

> Signed-off-by: Torstein Hegge <hegge@resisty.net>
> ---
> I don't know how useful the added test is, I didn't find any existing
> tests that looks at the comment parts of bisect log.

Thanks for adding a test. It's always appreciated.

Show 16 quoted lines

>  git-bisect.sh               |    8 +++++++-
>  t/t6030-bisect-porcelain.sh |   18 ++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> 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.

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

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

Otherwise this patch looks good to me.

> +		bad_commit=$(git show-branch $bad_rev)
> +		echo "# first bad commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
> +		exit 0
> +	fi

Thanks, Christian.