Bug 602158 - git bisect calls gitk without checking whether it is available
Summary: git bisect calls gitk without checking whether it is available
Alias: None
Product: Fedora
Classification: Fedora
Component: git
Version: 14
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Adam Tkac
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2010-06-09 09:38 UTC by Dhaval Giani
Modified: 2013-01-10 10:42 UTC (History)
7 users (show)

Fixed In Version: git-
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2011-04-05 21:35:10 UTC

Attachments (Terms of Use)

Description Dhaval Giani 2010-06-09 09:38:14 UTC
[dhaval@gondor libcg]$ git bisect visualize
/usr/libexec/git-core/git-bisect: line 303: gitk: command not found
git: 'bisect' is not a git command. See 'git --help'.

Did you mean this?

As can be seen, git bisect depends on gitk, so please make the appropriate git package (git-core in this case if i am correct) have a dependency on gitk.

[dhaval@gondor tmp]$ rpm -qv git
[dhaval@gondor tmp]$

Comment 1 Josh Boyer 2010-06-09 12:18:04 UTC
I'd rather not have git pull in gitk and tk if we can avoid it.  Might not be possible though.

Comment 2 Adam Tkac 2010-06-09 12:52:54 UTC
Actually there are two ways how to solve this issue:

1. add a distribution-specific patch to git-bisect to check if gitk is present. If no then write something like "please install gitk package to get this functionality working".

2. simply add Requires: gitk

In my opinion we can safely choose the second way because gitk has only tcl and tk packages as "non-standard dependencies". Tcl and tk depends on "standard dependencies" only, like libc and libX11 which are present on vast majority of systems. Per `yum install gitk` gitk + tk + tcl needs only 8.3 M of additional space which is not a big issue nowadays.

Comment 3 Todd Zullinger 2010-06-09 13:33:03 UTC
Hmm, gitk requires git, so making git require gitk might be fun.  I haven't woken up enough to think much about it yet.

My first thought is that the bisect script ought to check that gitk exists before attempting to execute it.  Then it could warn the user that gitk is needed for this one feature.  I must admit, I've used bisect a lot of times and never missed gitk.  And seeing that visualize has been around for 2 or 3 years and we've not had any bug reports so far, it seems not many others miss it either.

The code in question in git-bisect looks like this (squashed for space):

bisect_visualize() {
    bisect_next_check fail

    if test $# = 0
        case "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" in
        '')     set git log ;;
        set*)   set gitk ;;
        case "$1" in
        git*|tig) ;;
        -*)     set git log "$@" ;;
        *)      set git "$@" ;;

    eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")

Somewhere, a test for gitk's presence is needed I think. Though note that `which gitk` isn't portable across the many systems git runs on, and isn't used in any of the git tools which use shell, AFAIK.

Comment 4 Todd Zullinger 2010-06-09 13:35:36 UTC
Adam, why would we consider adding a distro specific patch?  If it's worth solving with a patch, it should be a patch that's something we can push upstream.

Comment 5 Todd Zullinger 2010-06-09 14:03:12 UTC
Further, requiring gitk will pull in libX11.  There are certainly folks who install git on servers where libX11 is not installed nor desired.  Making that a requirement for one small function in one of git's many tools seems like a bad trade to me.

Anyone with the itch to scratch is encouraged to suggest an upstream-able patch to check for gitk before using it in the bisect script.  Bonus points if you suggest it directly to the git list, following the patch submission guidelines in http://repo.or.cz/w/git.git?a=blob;f=Documentation/SubmittingPatches;hb=HEAD

Comment 6 Adam Tkac 2010-06-09 15:20:15 UTC
I proposed distro specific patch because it will be pretty simple, just something like `which gitk` or `rpm -q gitk`. It won't be accepted by upstream, of course. \

Although generic patch must be sophisticated enough, I think searching for "gitk" in $PATH should be sufficient and portable.

Comment 7 Ondrej Vasik 2010-06-10 06:55:46 UTC
I would suggest the similar way which is used by autoconf ( e.g. expanded macro AC_PATH_PROG() ) ... it should be portable and sophisticated enough ...

Comment 8 Adam Tkac 2011-03-28 14:46:30 UTC
This issue won't be fixed in Fedora 13, moving to Fedora 14.

Comment 9 Fedora Update System 2011-03-28 14:53:45 UTC
git- has been submitted as an update for Fedora 15.

Comment 10 Fedora Update System 2011-03-28 14:54:03 UTC
git- has been submitted as an update for Fedora 14.

Comment 11 Fedora Update System 2011-03-28 19:19:26 UTC
Package git-
* should fix your issue,
* was pushed to the Fedora 14 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing git-'
as soon as you are able to.
Please go to the following url:
then log in and leave karma (feedback).

Comment 12 Fedora Update System 2011-04-05 21:34:53 UTC
git- has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 Fedora Update System 2011-04-06 22:36:07 UTC
git- has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.