gitk: to run in a bare repository (was: gitk can't be run from non-worktree folders)
diff mbox series

Message ID xmqqk15i3rp7.fsf_-_@gitster-ct.c.googlers.com
State New
Headers show
Series
  • gitk: to run in a bare repository (was: gitk can't be run from non-worktree folders)
Related show

Commit Message

Junio C Hamano Jan. 23, 2020, 7:20 p.m. UTC
[This message is designed to be readable by "git am -c".]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> It's a new regression introduced by 2d92ab32fd (rev-parse: make
>> --show-toplevel without a worktree an error, 2019-11-19), as far as I
>> can tell. I have many times used gitk on bare repositories as an
>> interactive replacement for git-log, so this is a unfortunate bit of
>> fallout from that change. That's not to say that 2d92ab32fd should be
>> reverted, though... perhaps gitk itself needs a bit of a fix.
>
> Curious.
>
> There is a "proc gitworktree" that does use --show-toplevel but it
> is fairly conservative and assumes these calls can fail.
>
>     ...
>     if {[catch {set _gitworktree [exec git rev-parse --show-toplevel]}]} {
>         if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
> 	    catch {set _gitworktree [exec git config --get core.worktree]}
> 	    if {$_gitworktree eq ""} {
> 		set _gitworktree [file normalize ./[exec git rev-parse --show-cdup]]
> 	    }
>
> However, there is this call
>
>     set worktree [exec git rev-parse --show-toplevel]
>
> at the top-level of the code.  I wonder if the obvious and minimum
> fix would be sufficient, i.e.
>
>     -set worktree [exec git rev-parse --show-toplevel]
>     +set worktree [gitworktree]
>
> around l.12546

I've read 784b7e2f ("gitk: Fix "External diff" with separate work
tree", 2011-04-04) again and am reasonably convinced that the above
single liner would be "sufficient".  The external_diff_get_one_file
proc is written, with or without the "fix external diff" patch, to
work ONLY in a non-bare repository (it used to find and use the
parent directory of ".git" as the top-level of the working tree
before 784b7e2f, which changed it to use $worktree global obtained
by calling "rev-parse --show-toplevel") and does not work in a bare
repository anyway, if I am reading the code correctly.


-- >8 --
Subject: [PATCH] gitk: be prepared to be run in a bare repository

784b7e2f ("gitk: Fix "External diff" with separate work tree",
2011-04-04) added an unconditional call to "git rev-parse
--show-toplevel" to set up a global variable quite early in the
course of the program, so that the location of the working tree can
later be known if/when the user chooses to run the external diff via
the external_diff_get_one_file proc.  Before that change, the
external diff code used to assume that the parent directory of ".git"
directory is the top-level of the working tree.

Recent versions of git however notices that "rev-parse --show-toplevel"
executed in a bare repository is an error, which makes gitk stop,
even before the user could attempt to run external diff.

Use the gitworktree helper introduced in 65bb0bda ("gitk: Fix the
display of files when filtered by path", 2011-12-13), which is
prepared to see failures from "rev-parse --show-toplevel" and other
means it tries to find the top-level of the working tree instead to
work around this issue.  The resulting value in $worktree global,
when run in a bare repository, is bogus, but the code is not
prepared to run external diff correctly without a working tree
anyway ;-)

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sunshine Jan. 23, 2020, 7:26 p.m. UTC | #1
On Thu, Jan 23, 2020 at 11:20:36AM -0800, Junio C Hamano wrote:
> Use the gitworktree helper introduced in 65bb0bda ("gitk: Fix the
> display of files when filtered by path", 2011-12-13), which is
> prepared to see failures from "rev-parse --show-toplevel" and other
> means it tries to find the top-level of the working tree instead to
> work around this issue.  The resulting value in $worktree global,
> when run in a bare repository, is bogus, but the code is not
> prepared to run external diff correctly without a working tree
> anyway ;-)
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/gitk b/gitk
> @@ -12599,7 +12599,7 @@ set cdup {}
>  if {[expr {[exec git rev-parse --is-inside-work-tree] == "true"}]} {
>      set cdup [exec git rev-parse --show-cdup]
>  }
> -set worktree [exec git rev-parse --show-toplevel]
> +set worktree [gitworktree]

This helps but doesn't quite make it functional due to a bug in gitworktree() which results in:

    Error in startup script: can't read "_gitworktree": no such variable
        while executing
    "if {$_gitworktree eq ""} {

So, to make this work, it also needs:

--- >8 ---
diff --git a/gitk-git/gitk b/gitk-git/gitk
index abe4805ade..8cbca113e3 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -34,8 +34,7 @@ proc gitworktree {} {
         # cdup to obtain a relative path to the top of the worktree. If
         # run from the top, the ./ prefix ensures normalize expands pwd.
         if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
-	    catch {set _gitworktree [exec git config --get core.worktree]}
-	    if {$_gitworktree eq ""} {
+	    if {[catch {set _gitworktree [exec git config --get core.worktree]}]} {
 		set _gitworktree [file normalize ./[exec git rev-parse --show-cdup]]
 	    }
         }
--- >8 ---
Jeff King Jan. 23, 2020, 7:27 p.m. UTC | #2
On Thu, Jan 23, 2020 at 11:20:36AM -0800, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] gitk: be prepared to be run in a bare repository

Thanks all for cleaning up the mess I caused. I do prefer adjusting gitk
like this rather than reverting the change in Git. Despite the
regression, I think the case of "--show-toplevel without a working tree"
is sufficiently undefined that it's probably good for callers to
actually decide what the right behavior is.

> Recent versions of git however notices that "rev-parse --show-toplevel"
> executed in a bare repository is an error, which makes gitk stop,
> even before the user could attempt to run external diff.

It might be worth mentioning 2d92ab32 by name in this paragraph of the
commit message.

-Peff

Patch
diff mbox series

diff --git a/gitk b/gitk
index abe4805ade..1483bf5ed5 100755
--- a/gitk
+++ b/gitk
@@ -12599,7 +12599,7 @@  set cdup {}
 if {[expr {[exec git rev-parse --is-inside-work-tree] == "true"}]} {
     set cdup [exec git rev-parse --show-cdup]
 }
-set worktree [exec git rev-parse --show-toplevel]
+set worktree [gitworktree]
 setcoords
 makewindow
 catch {