{"thread":{"id":"31698","subject":"[PATCH] more meaningful error message in gitk when git binary is not available","startedAt":"2012-10-01T07:17:20Z","lastAt":"2012-10-01T17:24:01Z","messageCount":4,"participants":["Josef Assad","Joachim Schmitz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"200248","messageId":"50694380.4090108@josefassad.com","threadId":"31698","inReplyTo":null,"subject":"[PATCH] more meaningful error message in gitk when git binary is not available","fromName":"Josef Assad","fromEmail":"josef@josefassad.com","sentAt":"2012-10-01T07:17:20Z","receivedAt":"2012-10-01T07:17:20Z","isPatch":true,"sender":{"key":"josef@josefassad.com","avatar":null},"body":"Hi. I ran across what is a decidedly trivial little issue in gitk. The\nTCL/Tk looked simple enough so I am giving you a patch anyhow in case\nyou want to fix it.\n\nWhen for whatever reason the git binary is unavailable, gitk would\ncomplain about missing git repository instead, so this patch adds a\ncheck for git binary availability.\n\nIn case anyone is curious, I found this issue here:\n\nhttp://stackoverflow.com/q/11967110/53936\n\n\n\nSigned-off-by: Josef Assad <josef@josefassad.com>\n---\n gitk-git/gitk |    6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex d93bd99..7e2e0a7 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -11680,6 +11680,12 @@ setui $uicolor\n\n setoptions\n\n+# check that the git executables are available for use\n+if [catch {set gitexists [exec which git]}] {\n+    show_error {} . [mc \"Cannot find a suitable git executable.\"]\n+    exit 1\n+}\n+\n # check that we can find a .git directory somewhere...\n if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n     show_error {} . [mc \"Cannot find a git repository here.\"]\n-- \n1.7.5.4\n"},{"id":"200259","messageId":"k4c6en$qq$1@ger.gmane.org","threadId":"31698","inReplyTo":"50694380.4090108@josefassad.com","subject":"Re: [PATCH] more meaningful error message in gitk when git binary is not available","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-10-01T13:39:28Z","receivedAt":"2012-10-01T13:39:28Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Josef Assad wrote:\n> Hi. I ran across what is a decidedly trivial little issue in gitk. The\n> TCL/Tk looked simple enough so I am giving you a patch anyhow in case\n> you want to fix it.\n> \n> When for whatever reason the git binary is unavailable, gitk would\n> complain about missing git repository instead, so this patch adds a\n> check for git binary availability.\n> \n> In case anyone is curious, I found this issue here:\n> \n> http://stackoverflow.com/q/11967110/53936\n> \n> \n> \n> Signed-off-by: Josef Assad <josef@josefassad.com>\n> ---\n> gitk-git/gitk |    6 ++++++\n> 1 files changed, 6 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitk-git/gitk b/gitk-git/gitk\n> index d93bd99..7e2e0a7 100755\n> --- a/gitk-git/gitk\n> +++ b/gitk-git/gitk\n> @@ -11680,6 +11680,12 @@ setui $uicolor\n> \n> setoptions\n> \n> +# check that the git executables are available for use\n> +if [catch {set gitexists [exec which git]}] {\n\nI believe 'which' is not portable, you could use 'type' instead.\n\n> +    show_error {} . [mc \"Cannot find a suitable git executable.\"]\n> +    exit 1\n> +}\n> +\n> # check that we can find a .git directory somewhere...\n> if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n>     show_error {} . [mc \"Cannot find a git repository here.\"]\n"},{"id":"200266","messageId":"7va9w6dsnq.fsf@alter.siamese.dyndns.org","threadId":"31698","inReplyTo":"50694380.4090108@josefassad.com","subject":"Re: [PATCH] more meaningful error message in gitk when git binary is not available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T17:11:05Z","receivedAt":"2012-10-01T17:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josef Assad <josef@josefassad.com> writes:\n\n> Signed-off-by: Josef Assad <josef@josefassad.com>\n> ---\n>  gitk-git/gitk |    6 ++++++\n>  1 files changed, 6 insertions(+), 0 deletions(-)\n\nThanks.\n\n> diff --git a/gitk-git/gitk b/gitk-git/gitk\n> index d93bd99..7e2e0a7 100755\n> --- a/gitk-git/gitk\n> +++ b/gitk-git/gitk\n> @@ -11680,6 +11680,12 @@ setui $uicolor\n>\n>  setoptions\n>\n> +# check that the git executables are available for use\n> +if [catch {set gitexists [exec which git]}] {\n> +    show_error {} . [mc \"Cannot find a suitable git executable.\"]\n> +    exit 1\n> +}\n> +\n>  # check that we can find a .git directory somewhere...\n>  if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n>      show_error {} . [mc \"Cannot find a git repository here.\"]\n\nIt is somewhat a stupid solution to add an extra fork that will only\nwaste cycles in the normal non-error case, especially when we\nalready have an error codepath that acts on lack of the \"git\"\ncommand anyway, isn't it?\n\nThe \"rev-parse\" you see in the post-context will fail when we are\nnot in a git repository, but it will also fail when we do not have\ngit.\n\nYou can add the new check to if {[catch {... git rev-parse }]} block;\nbefore unconditionally saying \"cannot find a git repo\", you check if\n\"git\" even exists, and give an appropriate error message.  That way,\nyou do not punish normal use case with an extra useless fork.\n\nSomething like this, I presume.\n\n\n gitk | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex d93bd99..60794a7 100755\n--- a/gitk\n+++ b/gitk\n@@ -11682,7 +11682,12 @@ setoptions\n \n # check that we can find a .git directory somewhere...\n if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n-    show_error {} . [mc \"Cannot find a git repository here.\"]\n+    # we could have failed because there is no git to begin with\n+    if {[catch {exec git version}]} {\n+        show_error {} . [mc \"You do not seem to have 'git' command.\"]\n+    } else {\n+        show_error {} . [mc \"Cannot find a git repository here.\"]\n+    }\n     exit 1\n }\n \n"},{"id":"200268","messageId":"5069D1B1.3030703@josefassad.com","threadId":"31698","inReplyTo":"5069D0FE.4000609@josefassad.com","subject":"Re: [PATCH] more meaningful error message in gitk when git binary is not available","fromName":"Josef Assad","fromEmail":"josef@josefassad.com","sentAt":"2012-10-01T17:24:01Z","receivedAt":"2012-10-01T17:24:01Z","isPatch":true,"sender":{"key":"josef@josefassad.com","avatar":null},"body":"On 10/01/2012 07:21 PM, Josef Assad wrote:\n> On 10/01/2012 07:11 PM, Junio C Hamano wrote:\n>> Josef Assad <josef@josefassad.com> writes:\n>>\n>>> Signed-off-by: Josef Assad <josef@josefassad.com>\n>>> ---\n>>>  gitk-git/gitk |    6 ++++++\n>>>  1 files changed, 6 insertions(+), 0 deletions(-)\n>>\n>> Thanks.\n>>\n>>> diff --git a/gitk-git/gitk b/gitk-git/gitk\n>>> index d93bd99..7e2e0a7 100755\n>>> --- a/gitk-git/gitk\n>>> +++ b/gitk-git/gitk\n>>> @@ -11680,6 +11680,12 @@ setui $uicolor\n>>>\n>>>  setoptions\n>>>\n>>> +# check that the git executables are available for use\n>>> +if [catch {set gitexists [exec which git]}] {\n>>> +    show_error {} . [mc \"Cannot find a suitable git executable.\"]\n>>> +    exit 1\n>>> +}\n>>> +\n>>>  # check that we can find a .git directory somewhere...\n>>>  if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n>>>      show_error {} . [mc \"Cannot find a git repository here.\"]\n>>\n>> It is somewhat a stupid solution to add an extra fork that will only\n>> waste cycles in the normal non-error case, especially when we\n>> already have an error codepath that acts on lack of the \"git\"\n>> command anyway, isn't it?\n> \n> I don't think it's actually _stupid_, but I also think your solution\n> works better if you're trying to avoid one more exec call.\n> \n> Mine has one less level of indentation though, and it has clearer\n> delineation between checking for and handling two distinct error\n> conditions. :)\n> \n>> The \"rev-parse\" you see in the post-context will fail when we are\n>> not in a git repository, but it will also fail when we do not have\n>> git.\n>>\n>> You can add the new check to if {[catch {... git rev-parse }]} block;\n>> before unconditionally saying \"cannot find a git repo\", you check if\n>> \"git\" even exists, and give an appropriate error message.  That way,\n>> you do not punish normal use case with an extra useless fork.\n>>\n>> Something like this, I presume.\n>>\n>>\n>>  gitk | 7 ++++++-\n>>  1 file changed, 6 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/gitk b/gitk\n>> index d93bd99..60794a7 100755\n>> --- a/gitk\n>> +++ b/gitk\n>> @@ -11682,7 +11682,12 @@ setoptions\n>>  \n>>  # check that we can find a .git directory somewhere...\n>>  if {[catch {set gitdir [exec git rev-parse --git-dir]}]} {\n>> -    show_error {} . [mc \"Cannot find a git repository here.\"]\n>> +    # we could have failed because there is no git to begin with\n>> +    if {[catch {exec git version}]} {\n>> +        show_error {} . [mc \"You do not seem to have 'git' command.\"]\n>> +    } else {\n>> +        show_error {} . [mc \"Cannot find a git repository here.\"]\n>> +    }\n>>      exit 1\n>>  }\n> \n> I'm neutral though and not married to my patch in any way, just trying\n> to be helpful. In my opinon, yours is cleaner, mine is a tiny bit more\n> readable in a file already closing on 12k lines.\n> \n\nBah, forgot to Cc list. My bad.\n"}]}