[PATCH 1/3] bisect: improve error message of 'bisect log' while not bisecting

[PATCH 1/3] bisect: improve error message of 'bisect log' while not bisecting

[SZEDER Gábor]

'git bisect log' is implemented by a direct invocation of 'cat
"$GIT_DIR/BISECT_LOG"', without any sanity checks.  Consequently,
running 'git bisect log' while not bisecting leads to an error,
because the bisect logfile doesn't exists.  The accompanying error
message

  cat: /path/to/repo/.git/BISECT_LOG: No such file or directory

is neither very helpful nor very friendly.

Instead of blindly trying to cat the log file, first check whether
there is a bisection going on (i.e. the bisect logfile exists), and
die with a more appropriate error message when not.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 git-bisect.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 6e2acb8..3a4bf81 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -412,6 +412,10 @@ bisect_run () {
     done
 }
 
+bisect_log () {
+	test -s "$GIT_DIR/BISECT_LOG" || die "We are not bisecting."
+	cat "$GIT_DIR/BISECT_LOG"
+}
 
 case "$#" in
 0)
@@ -438,7 +442,7 @@ case "$#" in
     replay)
 	bisect_replay "$@" ;;
     log)
-	cat "$GIT_DIR/BISECT_LOG" ;;
+	bisect_log ;;
     run)
         bisect_run "$@" ;;
     *)
-- 
1.7.3.1.148.g2fffa

[PATCH 2/3] bisect: improve error msg of 'bisect reset' when original HEAD is deleted

[SZEDER Gábor]

'git bisect reset' (without the optional <commit> argument) returns to
the original HEAD from where the bisection was started.  However,
when, for whatever reason, the user deleted the original HEAD before
invoking 'git bisect reset', then all he gets is an error message from
'git checkout':

  fatal: invalid reference: somebranch

Let's try to be more helpful with an error message better describing
what went wrong and a suggestion about how to resolve the situation:

  Could not check out original HEAD 'somebranch'. Try 'git bisect reset <commit>'.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 git-bisect.sh |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 3a4bf81..68fcff6 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -316,7 +316,12 @@ bisect_reset() {
 	*)
 	    usage ;;
 	esac
-	git checkout "$branch" -- && bisect_clean_state
+	if git checkout "$branch" -- ; then
+		bisect_clean_state
+	else
+		die "Could not check out original HEAD '$branch'." \
+				"Try 'git bisect reset <commit>'."
+	fi
 }
 
 bisect_clean_state() {
-- 
1.7.3.1.148.g2fffa

[PATCH 3/3] bisect: check for mandatory argument of 'bisect replay'

[SZEDER Gábor]

'git bisect replay' has a mandatory logfile argument, but the current
implementation doesn't check whether the user has specified one.  When
the user omits the logfile argument, this leads to the following
unhelpful error message:

  cannot read  for replaying

So, check for the mandatory argument first, and provide a more
meaningful error message when it is omitted.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 git-bisect.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 68fcff6..c21e33c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -343,6 +343,7 @@ bisect_clean_state() {
 }
 
 bisect_replay () {
+	test "$#" -eq 1 || die "No logfile given"
 	test -r "$1" || die "cannot read $1 for replaying"
 	bisect_reset
 	while read git bisect command rev
-- 
1.7.3.1.148.g2fffa

Re: [PATCH 3/3] bisect: check for mandatory argument of 'bisect replay'

[Christian Couder]

On Sunday 10 October 2010 23:48:58 SZEDER Gábor wrote:
> 'git bisect replay' has a mandatory logfile argument, but the current
> implementation doesn't check whether the user has specified one.  When
> the user omits the logfile argument, this leads to the following
> unhelpful error message:
> 
>   cannot read  for replaying
> 
> So, check for the mandatory argument first, and provide a more
> meaningful error message when it is omitted.
> 
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
>  git-bisect.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 68fcff6..c21e33c 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -343,6 +343,7 @@ bisect_clean_state() {
>  }
> 
>  bisect_replay () {
> +	test "$#" -eq 1 || die "No logfile given"
>  	test -r "$1" || die "cannot read $1 for replaying"
>  	bisect_reset
>  	while read git bisect command rev

While at it perhaps you could do something like:

bisect_replay () {
+	test "$#" -lt 1 || die "No logfile given"
+	test "$#" -gt 1 || die "Too many argument. Please give only one logfile."
 	test -r "$1" || die "cannot read $1 for replaying"
 	bisect_reset
 	while read git bisect command rev

Thanks,
Christian.

Re: [PATCH 3/3] bisect: check for mandatory argument of 'bisect replay'

[Christian Couder]

On Tuesday 12 October 2010 04:35:11 Christian Couder wrote:
> On Sunday 10 October 2010 23:48:58 SZEDER Gábor wrote:
> > 'git bisect replay' has a mandatory logfile argument, but the current
> > implementation doesn't check whether the user has specified one.  When
> > the user omits the logfile argument, this leads to the following
> > 
> > unhelpful error message:
> >   cannot read  for replaying
> > 
> > So, check for the mandatory argument first, and provide a more
> > meaningful error message when it is omitted.
> > 
> > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> > ---
> > 
> >  git-bisect.sh |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index 68fcff6..c21e33c 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -343,6 +343,7 @@ bisect_clean_state() {
> > 
> >  }
> >  
> >  bisect_replay () {
> > 
> > +	test "$#" -eq 1 || die "No logfile given"
> > 
> >  	test -r "$1" || die "cannot read $1 for replaying"
> >  	bisect_reset
> >  	while read git bisect command rev
> 
> While at it perhaps you could do something like:
> 
> bisect_replay () {
> +	test "$#" -lt 1 || die "No logfile given"
> +	test "$#" -gt 1 || die "Too many argument. Please give only one logfile."
>  	test -r "$1" || die "cannot read $1 for replaying"
>  	bisect_reset
>  	while read git bisect command rev

I mean:

bisect_replay () {
+	test "$#" -lt 1 && die "No logfile given"
+	test "$#" -gt 1 && die "Too many argument. Please give only one logfile."
 	test -r "$1" || die "cannot read $1 for replaying"
 	bisect_reset
 	while read git bisect command rev

Sorry!

Best regards,
Christian.

Re: [PATCH 3/3] bisect: check for mandatory argument of 'bisect replay'

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> >  bisect_replay () {
>> > 
>> > +	test "$#" -eq 1 || die "No logfile given"
>> > 
>> >  	test -r "$1" || die "cannot read $1 for replaying"
>> >  	bisect_reset
>> >  	while read git bisect command rev
>> 
>> While at it perhaps you could do something like:
>> 
>> bisect_replay () {
>> +	test "$#" -lt 1 || die "No logfile given"
>> +	test "$#" -gt 1 || die "Too many argument. Please give only one logfile."
>>  	test -r "$1" || die "cannot read $1 for replaying"
>>  	bisect_reset
>>  	while read git bisect command rev
>
> I mean:
>
> bisect_replay () {
> +	test "$#" -lt 1 && die "No logfile given"
> +	test "$#" -gt 1 && die "Too many argument. Please give only one logfile."
>  	test -r "$1" || die "cannot read $1 for replaying"
>  	bisect_reset
>  	while read git bisect command rev

That suggests that the original patch is buggy as well, in that it says
"No logfile given" when it gets two.

How about checking with 1 and saying "Give me only one logfile" and be
done with it?

Re: [PATCH 3/3] bisect: check for mandatory argument of 'bisect replay'

[Christian Couder]

On Wed, Oct 13, 2010 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>>> >  bisect_replay () {
>>> >
>>> > +  test "$#" -eq 1 || die "No logfile given"
>>> >
>>> >    test -r "$1" || die "cannot read $1 for replaying"
>>> >    bisect_reset
>>> >    while read git bisect command rev
>>>
>>> While at it perhaps you could do something like:
>>>
>>> bisect_replay () {
>>> +    test "$#" -lt 1 || die "No logfile given"
>>> +    test "$#" -gt 1 || die "Too many argument. Please give only one logfile."
>>>      test -r "$1" || die "cannot read $1 for replaying"
>>>      bisect_reset
>>>      while read git bisect command rev
>>
>> I mean:
>>
>> bisect_replay () {
>> +     test "$#" -lt 1 && die "No logfile given"
>> +     test "$#" -gt 1 && die "Too many argument. Please give only one logfile."
>>       test -r "$1" || die "cannot read $1 for replaying"
>>       bisect_reset
>>       while read git bisect command rev
>
> That suggests that the original patch is buggy as well, in that it says
> "No logfile given" when it gets two.
>
> How about checking with 1 and saying "Give me only one logfile" and be
> done with it?

Yeah or: "Give me exactly one logfile as argument"

Best regards,
Christian.

Re: [PATCH 1/3] bisect: improve error message of 'bisect log' while not bisecting

[Ævar Arnfjörð Bjarmason]

2010/10/10 SZEDER Gábor <szeder@ira.uka.de>:
> 'git bisect log' is implemented by a direct invocation of 'cat
> "$GIT_DIR/BISECT_LOG"', without any sanity checks.  Consequently,
> running 'git bisect log' while not bisecting leads to an error,
> because the bisect logfile doesn't exists.  The accompanying error
> message

Nice, in case you want to fix this for git-rebase too:

Applying: gettextize: git-add "no files added" message
Applying: gettextize: git-add "Use -f if you really want" message
Applying: gettextize: git-add "pathspec [...] did not match" message
Applying: gettextize: git-add "remove '%s'" message
^C
v t ((5cab536...)|REBASE) $ git rebase --abort
cat: /home/avar/g/git/.git/rebase-apply/head-name: No such file or directory
cat: /home/avar/g/git/.git/rebase-apply/orig-head: No such file or directory
HEAD is now at 5cab536 gettextize: git-add "remove '%s'" message
v t ((5cab536...)) $
v t ((5cab536...)) $ git rebase --abort
No rebase in progress?
v t ((5cab536...)) $