From: Junio C Hamano <hidden> Date: 2020-01-23 19:20:47
[This message is designed to be readable by "git am -c".]
Junio C Hamano [off-list ref] writes:
Eric Sunshine [off-list ref] writes:
quoted
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 [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
gitk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Eric Sunshine <hidden> Date: 2020-01-23 19:26:47
On Thu, Jan 23, 2020 at 11:20:36AM -0800, Junio C Hamano wrote:
quoted hunk
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 [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
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:
@@ -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 ---
From: Jeff King <hidden> Date: 2020-01-23 19:27:10
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
Sorry for bumping this thread but what's the integration status of the patch?
The issue still seems to be present in v2.26.0.windows.1.
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
From: Eric Sunshine <hidden> Date: 2020-10-11 05:09:00
On Mon, Mar 30, 2020 at 11:21 AM ch [off-list ref] wrote:
On Thu, Jan 23, 2020 at 11:20:36AM -0800, Junio C Hamano wrote:
> Subject: [PATCH] gitk: be prepared to be run in a bare repository
Sorry for bumping this thread but what's the integration status of the patch?
The issue still seems to be present in v2.26.0.windows.1.
Junio just recently pulled commits into git.git from Paul's gitk
repository which contain this fix, and it looks like it will make it
into the Git 2.29.0 release.
On Mon, Mar 30, 2020 at 11:21 AM ch [off-list ref] wrote:
quoted
On Thu, Jan 23, 2020 at 11:20:36AM -0800, Junio C Hamano wrote:
> Subject: [PATCH] gitk: be prepared to be run in a bare repository
Sorry for bumping this thread but what's the integration status of the patch?
The issue still seems to be present in v2.26.0.windows.1.
Junio just recently pulled commits into git.git from Paul's gitk
repository which contain this fix, and it looks like it will make it
into the Git 2.29.0 release.