Re: [PATCH ] "git bisect visualize" results in an invalid error if "gitk" is not installed
- From
- Maxin john <maxin@maxinbjohn.info>
- Date
- Mar 21, 2011, 12:26 UTC
- 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:
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