From: Maxin john <maxin@maxinbjohn.info>
Date: Mon, 21 Mar 2011 12:26:33 GMT
Subject: Re: [PATCH ] "git bisect visualize" results in an invalid error if "gitk" is not installed
Message-ID: <AANLkTinS87obXcgbcFZ8L-UjZUQL96SzpHp84Y6-yX6v@mail.gmail.com>
In-Reply-To: <20110321112932.GF16334@sigill.intra.peff.net>

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

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

> 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