[REGRESSION] gitk can't be run from non-worktree folders

[REGRESSION] gitk can't be run from non-worktree folders

[ch]

Hi all!

I recently updated my Git installation from 2.24.1.windows.2 to
2.25.0.windows.1 and from the looks of it the included version of gitk does not
support being run from non-worktree folders of a repository anymore (like the
.git folder for example).

Here's a small reproduction recipe:

   $ git init repro
   $ cd repro
   $ git commit -m test --allow-empty
   $ cd .git
   $ gitk --all &

gitk terminates with the message:

   fatal: this operation must be run in a work tree
       while executing
   "exec git rev-parse --show-toplevel"
       invoked from within
   "set worktree [exec git rev-parse --show-toplevel]"
       (file "C:/Program Files/Git/mingw64/bin/gitk" line 12634)

-ch

Re: [REGRESSION] gitk can't be run from non-worktree folders

[SZEDER Gábor]

On Thu, Jan 23, 2020 at 05:04:09PM +0100, ch wrote:
> I recently updated my Git installation from 2.24.1.windows.2 to
> 2.25.0.windows.1 and from the looks of it the included version of gitk does not
> support being run from non-worktree folders of a repository anymore (like the
> .git folder for example).
> 
> Here's a small reproduction recipe:
> 
>   $ git init repro
>   $ cd repro
>   $ git commit -m test --allow-empty
>   $ cd .git
>   $ gitk --all &
> 
> gitk terminates with the message:
> 
>   fatal: this operation must be run in a work tree
>       while executing
>   "exec git rev-parse --show-toplevel"
>       invoked from within
>   "set worktree [exec git rev-parse --show-toplevel]"
>       (file "C:/Program Files/Git/mingw64/bin/gitk" line 12634)

I've stumbled upon the same issue when I tried to run 'gitk' in a bare
repository a while ago, but it refused to run with the same error
message.

Are you sure it's a recent regression?  I have no experience with
'gitk' in Git for Windows, but on Linux this has been broken for quite
some time, namely since 784b7e2f25 (gitk: Fix "External diff" with
separate work tree, 2011-04-04) in the v1.7 era.  FWIW, reverting that
commit on top of v2.25.0 makes 'gitk' work again in bare repositories.

Re: [REGRESSION] gitk can't be run from non-worktree folders

[Eric Sunshine]

[cc:+peff]

On Thu, Jan 23, 2020 at 11:32 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Jan 23, 2020 at 05:04:09PM +0100, ch wrote:
> > I recently updated my Git installation from 2.24.1.windows.2 to
> > 2.25.0.windows.1 and from the looks of it the included version of gitk does not
> > support being run from non-worktree folders of a repository anymore (like the
> > .git folder for example).
> I've stumbled upon the same issue when I tried to run 'gitk' in a bare
> repository a while ago, but it refused to run with the same error
> message.
>
> Are you sure it's a recent regression?  I have no experience with
> 'gitk' in Git for Windows, but on Linux this has been broken for quite
> some time, namely since 784b7e2f25 (gitk: Fix "External diff" with
> separate work tree, 2011-04-04) in the v1.7 era.  FWIW, reverting that
> commit on top of v2.25.0 makes 'gitk' work again in bare repositories.

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.

Re: [REGRESSION] gitk can't be run from non-worktree folders

[SZEDER Gábor]

On Thu, Jan 23, 2020 at 11:36:49AM -0500, Eric Sunshine wrote:
> [cc:+peff]
> 
> On Thu, Jan 23, 2020 at 11:32 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Thu, Jan 23, 2020 at 05:04:09PM +0100, ch wrote:
> > > I recently updated my Git installation from 2.24.1.windows.2 to
> > > 2.25.0.windows.1 and from the looks of it the included version of gitk does not
> > > support being run from non-worktree folders of a repository anymore (like the
> > > .git folder for example).
> > I've stumbled upon the same issue when I tried to run 'gitk' in a bare
> > repository a while ago, but it refused to run with the same error
> > message.
> >
> > Are you sure it's a recent regression?  I have no experience with
> > 'gitk' in Git for Windows, but on Linux this has been broken for quite
> > some time, namely since 784b7e2f25 (gitk: Fix "External diff" with
> > separate work tree, 2011-04-04) in the v1.7 era.  FWIW, reverting that
> > commit on top of v2.25.0 makes 'gitk' work again in bare repositories.
> 
> 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.

Oh, that's interesting.

My local commit reverting 784b7e2f25 that I build my Git with is newer
than 2d92ab32fd, so maybe that's why I haven't recalled seeing this
issue earlier.  I did try to bisect it, but had a real hard time with
gitk being subtree merged, and ultimately gave up on that, and started
to look for a commit adding '--show-toplevel' to 'gitk', which lead me
to 784b7e2f25, I reverted it, my issue went away, which then lead me
to the wrong conclusion.

> 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.

Well, I don't speak much tcl/tk, but as far as I understand the
changes of 784b7e2f25, it added a 'git rev-parse --show-toplevel'
invocation to 'gitk's main code path, but the result of that command
is only used if the user clicks on the "External diff" menuitem in the
"Patch" pane's context menu.  So perhaps the scope of that 'git
rev-parse --show-toplevel' could be limited, and executed only when
the user actually clicks on that menuitem (I've never done that
before, in fact I wasn't even aware that there is a context menu
there).

Re: [REGRESSION] gitk can't be run from non-worktree folders

[SZEDER Gábor]

On Thu, Jan 23, 2020 at 06:04:12PM +0100, SZEDER Gábor wrote:
> On Thu, Jan 23, 2020 at 11:36:49AM -0500, Eric Sunshine wrote:

> > > Are you sure it's a recent regression?  I have no experience with
> > > 'gitk' in Git for Windows, but on Linux this has been broken for quite
> > > some time, namely since 784b7e2f25 (gitk: Fix "External diff" with
> > > separate work tree, 2011-04-04) in the v1.7 era.  FWIW, reverting that
> > > commit on top of v2.25.0 makes 'gitk' work again in bare repositories.
> > 
> > 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.
> 
> Oh, that's interesting.
> 
> My local commit reverting 784b7e2f25 that I build my Git with is newer
> than 2d92ab32fd, so maybe that's why I haven't recalled seeing this
> issue earlier.  I did try to bisect it, but had a real hard time with
> gitk being subtree merged, and ultimately gave up on that, and started
> to look for a commit adding '--show-toplevel' to 'gitk', which lead me
> to 784b7e2f25, I reverted it, my issue went away, which then lead me
> to the wrong conclusion.

Oh, this is interesting indeed :)

I have a couple of older Git versions lying around, so I can quickly
ballpark a revision range for a regression, without checking out and
building older versions.  This has always worked well so far, because
Git looks for its non-builtin commands in the hardcoded exec-path that
was specified at build time, while builtin commands are, well,
builtins.  Alas, 'gitk' is not a regular Git command, and it runs
whatever 'git' executable comes first in PATH, which in my case was a
recent build already containing 2d92ab32fd.

  $ ~/src/git/BUILDS/v1.8.0/bin/git --exec-path
  /home/szeder/src/git/BUILDS/v1.8.0/libexec/git-core
  $ cd .git
  $ ~/src/git/BUILDS/v1.8.0/bin/gitk
  Error in startup script: fatal: this operation must be run in a work
  tree
      while executing
  "exec git rev-parse --show-toplevel"
      invoked from within
  "set worktree [exec git rev-parse --show-toplevel]"
      (file "/home/szeder/src/git/BUILDS/v1.8.0/bin/gitk" line 11810)
  $ PATH=~/src/git/BUILDS/v1.8.0/bin:"$PATH" ~/src/git/BUILDS/v1.8.0/bin/gitk
  # Works even in the .git directory!
  $ PATH=~/src/git/BUILDS/v2.24.0/bin:"$PATH" ~/src/git/BUILDS/v2.24.0/bin/gitk
  # This works, too!

With this newly gained knowledge I added 'bin-wrappers' to $PATH when
running gitk built from 2d92ab32fd and its parent, and indeed that's
the commit that introduced this regression.

Re: [REGRESSION] gitk can't be run from non-worktree folders

[Junio C Hamano]

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

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

[Junio C Hamano]

[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(-)

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 {

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

[Eric Sunshine]

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 ---

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

[Jeff King]

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

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

[ch]

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

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

[Eric Sunshine]

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.

Re: [PATCH] gitk: to run in a bare repository

[ch]

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.
> 

Re: [PATCH] gitk: to run in a bare repository

[Pratyush Yadav]

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.
>> 
>

-- 
Regards,
Pratyush Yadav

Re: [REGRESSION] gitk can't be run from non-worktree folders

[ch]

I haven't actually bisected it but the change possibly also broke
git-gui-browser on bare repositories.

The follow patch seems to fix the issue. As I stated in my original bug report
I'm using Git for Windows so maybe it does not apply cleanly on upstream Git.

-- >8 --

--- git-gui.tcl	2020-03-23 12:22:36.000000000 +0100
+++ git-gui-fixed.tcl	2020-04-01 17:35:26.092223600 +0200
@@ -1286,7 +1286,7 @@
  	if { [is_Cygwin] } {
  		catch {set _gitworktree [exec cygpath --windows [git rev-parse --show-toplevel]]}
  	} else {
-		set _gitworktree [git rev-parse --show-toplevel]
+		catch {set _gitworktree [git rev-parse --show-toplevel]}
  	}
  } else {
  	# try to set work tree from environment, core.worktree or use

-- >8 --

On 23.01.2020 17:36, Eric Sunshine wrote:

> 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.
> 

Re: [REGRESSION] gitk can't be run from non-worktree folders

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> [cc:+peff]
>
> On Thu, Jan 23, 2020 at 11:32 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> On Thu, Jan 23, 2020 at 05:04:09PM +0100, ch wrote:
>> > I recently updated my Git installation from 2.24.1.windows.2 to
>> > 2.25.0.windows.1 and from the looks of it the included version of gitk does not
>> > support being run from non-worktree folders of a repository anymore (like the
>> > .git folder for example).
>> I've stumbled upon the same issue when I tried to run 'gitk' in a bare
>> repository a while ago, but it refused to run with the same error
>> message.
>>
>> Are you sure it's a recent regression?  I have no experience with
>> 'gitk' in Git for Windows, but on Linux this has been broken for quite
>> some time, namely since 784b7e2f25 (gitk: Fix "External diff" with
>> separate work tree, 2011-04-04) in the v1.7 era.  FWIW, reverting that
>> commit on top of v2.25.0 makes 'gitk' work again in bare repositories.
>
> 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.

I thought we already saw a patch to do so?

    https://lore.kernel.org/git/20200123192638.GA3177@flurp.local/

perhaps?

Paul?