threads / patch / 26807

patch"git bisect visualize" results in an invalid error if "gitk" is not installed

Subject: [PATCH ] "git bisect visualize" results in an invalid error if "gitk" is not installed

## tl;dr

4 messages between Mar 20, 2011 and Mar 21, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Maxin john· Mar 20, 2011, 21:10 UTC · lore
Hi,

While using "git bisect visualize" on my PC running Ubuntu 10.10, I came across this error:

$ git bisect visualize eval: 1: gitk: not found git: 'bisect' is not a git command. See 'git --help'.

Did you mean this?
	bisect
$

As this distribution don't keep "gitk" as a dependency for git,we will have to install "gitk" as a separate package. However, I found the error message a bit confusing. So,I have prepared a "quick and dirty" patch to solve it. Please let me know your comments.

Signed-off-by: Maxin B. John <maxin@maxinbjohn.info>
---
Show changes to 2 files +10 −1

git-bisect.sh, git-sh-setup.sh

diff --git a/git-bisect.sh b/git-bisect.sh
index c21e33c..fefe212 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -290,7 +290,8 @@ bisect_visualize() {
        then
                case
"${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}"
in
                '')     set git log ;;
-               set*)   set gitk ;;
+               set*)   is_gitk_present
+                       set gitk ;;
                esac
        else
                case "$1" in
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index aa16b83..5e78b54 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -132,6 +132,14 @@ is_bare_repository () {
        git rev-parse --is-bare-repository
 }

+is_gitk_present () {
+       GIT_GITK=$(which gitk)
+       test -n "$GIT_GITK" || {
+               echo >&2 "Cannot find 'gitk' in the PATH"
+               exit 1
+       }
+}
+
 cd_to_toplevel () {
        cdup=$(git rev-parse --show-toplevel) &&
        cd "$cdup" || {
Jeff King· Mar 21, 2011, 11:29 UTC · re: Maxin john · lore

Re: [PATCH ] "git bisect visualize" results in an invalid error if "gitk" is not installed

On Sun, Mar 20, 2011 at 11:10:55PM +0200, Maxin john wrote:
Show 10 quoted lines
> While using "git bisect visualize" on my PC running Ubuntu 10.10, I
> came across this error:
> 
> $ git bisect visualize
> eval: 1: gitk: not found
> git: 'bisect' is not a git command. See 'git --help'.
> 
> Did you mean this?
> 	bisect
> $
Yuck. Definitely non-optimal.
Show 14 quoted lines
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c21e33c..fefe212 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -290,7 +290,8 @@ bisect_visualize() {
>         then
>                 case
> "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}"
> in
>                 '')     set git log ;;
> -               set*)   set gitk ;;
> +               set*)   is_gitk_present
> +                       set gitk ;;
>                 esac

The point of this code is to use "gitk" if we can (i.e., if we have a grahpical display of some sort), and "git log" otherwise. Shouldn't "we are missing gitk" also cause us to fallback to using "git log"? IOW, something like:

  if test -n "${DISPLAY+set}..." && is_gitk_present; then
    set gitk
  else
    set git log
  fi
Show 7 quoted lines
> +is_gitk_present () {
> +       GIT_GITK=$(which gitk)
> +       test -n "$GIT_GITK" || {
> +               echo >&2 "Cannot find 'gitk' in the PATH"
> +               exit 1
> +       }
> +}

I don't think this is a portable use of which. In particular, I seem to recall SunOS which printing some junk to stderr like "no foo in /bin /usr/bin etc...". I think it even then returns a successful exit code, just to make it totally useless.

I think we tend to use the shell's "type" builtin for this, which has a usable exit code.

So the patch would look like:
Show changes to git-bisect.sh +6 −5
diff --git a/git-bisect.sh b/git-bisect.sh
index c21e33c..3b3156f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -288,10 +288,12 @@ bisect_visualize() {
 
 	if test $# = 0
 	then
-		case "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" in
-		'')	set git log ;;
-		set*)	set gitk ;;
-		esac
+		if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
+		   type gitk >/dev/null 2>&1; then
+			set gitk
+		else
+			set git log
+		fi
 	else
 		case "$1" in
 		git*|tig) ;;

but I didn't test it at all.

-Peff
Maxin john· Mar 21, 2011, 12:26 UTC · re: Jeff King · lore

Re: [PATCH ] "git bisect visualize" results in an invalid error if "gitk" is not installed

Hi Jeff,
I have tested the patch and it works like a charm!.
Tested-by: Maxin B. John <maxin@maxinbjohn.info>
On Mon, Mar 21, 2011 at 12:29 PM, Jeff King <peff@peff.net> wrote:
Show 41 quoted lines
> On Sun, Mar 20, 2011 at 11:10:55PM +0200, Maxin john wrote:
>
>> While using "git bisect visualize" on my PC running Ubuntu 10.10, I
>> came across this error:
>>
>> $ git bisect visualize
>> eval: 1: gitk: not found
>> git: 'bisect' is not a git command. See 'git --help'.
>>
>> Did you mean this?
>>       bisect
>> $
>
> Yuck. Definitely non-optimal.
>
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index c21e33c..fefe212 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -290,7 +290,8 @@ bisect_visualize() {
>>         then
>>                 case
>> "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}"
>> in
>>                 '')     set git log ;;
>> -               set*)   set gitk ;;
>> +               set*)   is_gitk_present
>> +                       set gitk ;;
>>                 esac
>
> The point of this code is to use "gitk" if we can (i.e., if we have a
> grahpical display of some sort), and "git log" otherwise. Shouldn't "we
> are missing gitk" also cause us to fallback to using "git log"? IOW,
> something like:
>
>  if test -n "${DISPLAY+set}..." && is_gitk_present; then
>    set gitk
>  else
>    set git log
>  fi
>
Yes. it is much better than just exiting if "gitk" is not present.
Show 15 quoted lines
>> +is_gitk_present () {
>> +       GIT_GITK=$(which gitk)
>> +       test -n "$GIT_GITK" || {
>> +               echo >&2 "Cannot find 'gitk' in the PATH"
>> +               exit 1
>> +       }
>> +}
>
> I don't think this is a portable use of which. In particular, I seem to
> recall SunOS which printing some junk to stderr like "no foo in /bin
> /usr/bin etc...". I think it even then returns a successful exit code,
> just to make it totally useless.
>
> I think we tend to use the shell's "type" builtin for this, which has a
> usable exit code.
"type" seems to be a better choice than using "which" for this case.
Show 32 quoted lines
> So the patch would look like:
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c21e33c..3b3156f 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -288,10 +288,12 @@ bisect_visualize() {
>
>        if test $# = 0
>        then
> -               case "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" in
> -               '')     set git log ;;
> -               set*)   set gitk ;;
> -               esac
> +               if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
> +                  type gitk >/dev/null 2>&1; then
> +                       set gitk
> +               else
> +                       set git log
> +               fi
>        else
>                case "$1" in
>                git*|tig) ;;
>
> but I didn't test it at all.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Warm Regards, Maxin B. John

Jeff King· Mar 21, 2011, 13:14 UTC · re: Maxin john · lore

[PATCH] bisect: visualize with git-log if gitk is unavailable

If gitk is not available in the PATH, bisect ends up exiting with the shell's 127 error code, confusing the git wrapper into thinking that bisect is not a git command.

We already fallback to git-log if there doesn't seem to be a graphical display available. We should do the same if gitk is not available in our PATH at all. This not only fixes the ugly error message, but is a much more sensible default than failing to show the user anything.

Reported by Maxin John.
Tested-by: Maxin B. John <maxin@maxinbjohn.info>
Signed-off-by: Jeff King <peff@peff.net>
---
 git-bisect.sh |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
Show changes to git-bisect.sh +6 −4
diff --git a/git-bisect.sh b/git-bisect.sh
index c21e33c..415a8d0 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -288,10 +288,12 @@ bisect_visualize() {
 
 	if test $# = 0
 	then
-		case "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" in
-		'')	set git log ;;
-		set*)	set gitk ;;
-		esac
+		if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
+		   type gitk >/dev/null 2>&1; then
+			set gitk
+		else
+			set git log
+		fi
 	else
 		case "$1" in
 		git*|tig) ;;

-- 
1.7.2.5.22.g853c5

← back to recent threads