Hi Jeff,
I have tested the patch and it works like a charm!.
Tested-by: Maxin B. John <maxin@xxxxxxxxxxxxxxx>
On Mon, Mar 21, 2011 at 12:29 PM, Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Warm Regards,
Maxin B. John
--
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