diff mbox series

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

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

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
ch March 30, 2020, 3:20 p.m. UTC | #3 
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
Eric Sunshine Oct. 11, 2020, 5:08 a.m. UTC | #4 
On Mon, Mar 30, 2020 at 11:21 AM ch <cr@onlinehome.de> 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.
ch Oct. 13, 2020, 2:25 p.m. UTC | #5 
Sounds good. Thanks for the heads-up!

Will the release also include a fix for git-gui (which exhibits
similar behavior as gitk; see [0])?

-ch

[0] https://lore.kernel.org/git/3c1a3e23-cf52-48cc-e9b6-f80642ca67ac@onlinehome.de/

On 11.10.2020 07:08, Eric Sunshine wrote:
> On Mon, Mar 30, 2020 at 11:21 AM ch <cr@onlinehome.de> 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.
>
Pratyush Yadav Oct. 13, 2020, 3:31 p.m. UTC | #6 
On Tue, Oct 13 2020, ch wrote:

Hi,

Please avoid top posting.

> Sounds good. Thanks for the heads-up!
>
> Will the release also include a fix for git-gui (which exhibits
> similar behavior as gitk; see [0])?
>
> -ch
>
> [0] https://lore.kernel.org/git/3c1a3e23-cf52-48cc-e9b6-f80642ca67ac@onlinehome.de/

I'm seeing this patch for the first time. It was never formally
submitted to me. So as of now I have not queued it for the next release.
I'll take a look at the problem and see if the patch is the correct fix
and integrate it before the 2.29 release. But I don't have a lot of free
time so no promises.

>
> On 11.10.2020 07:08, Eric Sunshine wrote:
>> On Mon, Mar 30, 2020 at 11:21 AM ch <cr@onlinehome.de> 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.
>> 
>
diff mbox series

Patch

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 {