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

[Jeff King <peff@xxxxxxxx>]

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

> +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:

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html