threads / patch / 17666

patch, 3 partsgit-gui: properly check for a bare repo

Subject: [PATCH 1/3] git-gui: properly check for a bare repo

## tl;dr

9 messages between Feb 9, 2009 and Feb 18, 2009. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Giuseppe Bilotta· Feb 9, 2009, 02:00 UTC · lore

[PATCH 0/3] git-gui: generic and robust worktree/gitdir support

A-hem.

<intro class="shameless plug"> I've recently restarted using (and thus working on) my Zit project, the Git-based single-file content tracker ( http://git.oblomov.eu/zit ), adding support for importing RCS history (via my rcs-fast-export project, http://git.oblomov.eu/rcs-fast-export ), and even managed to run a successful rebase (which turned out to be somewhat clumsier due to the inevitably longer command lines, but perfectly feasible). My next step was looking for ways to support the graphical utilities, in particular git-gui and gitk </intro>

It turns out that while gitk works pretty fine out of the box, git-gui makes a lot of guesses and assumptions about the nature and relation between the git dir and its worktree. The following three patches try to remedy the problems I came across with some simple testing.

Giuseppe Bilotta (3):
  git-gui: properly check for a bare repo
  git-gui: use the actual worktree
  git-gui: define correct GIT_DIR for gitk
Giuseppe Bilotta· Feb 9, 2009, 02:00 UTC · re: Giuseppe Bilotta · lore

When bare repository handling is not enabled, check for a bare repository looking at the core.bare config option rather than refusing to operate with a git directory ending with .git.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
I know I should have probably used something like
git rev-parse --is-bare-repository instead, but I didn't feel like
adding another git call. Is the config approach robust enough?
 git-gui/git-gui.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-gui/git-gui.sh +1 −1
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index e018e07..658a728 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1071,7 +1071,7 @@ if {$_prefix ne {}} {
 	}
 	unset cdup
 } elseif {![is_enabled bare]} {
-	if {[lindex [file split $_gitdir] end] ne {.git}} {
+	if {[is_config_true core.bare]} {
 		catch {wm withdraw .}
 		error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
 		exit 1

-- 
1.6.2.rc0.173.g5e148
Shawn O. Pearce· Feb 18, 2009, 17:16 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH 1/3] git-gui: properly check for a bare repo

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> wrote:
Show 23 quoted lines
> When bare repository handling is not enabled, check for a bare
> repository looking at the core.bare config option rather than refusing
> to operate with a git directory ending with .git.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> I know I should have probably used something like
> git rev-parse --is-bare-repository instead, but I didn't feel like
> adding another git call. Is the config approach robust enough?
> 
>  git-gui/git-gui.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index e018e07..658a728 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1071,7 +1071,7 @@ if {$_prefix ne {}} {
>  	}
>  	unset cdup
>  } elseif {![is_enabled bare]} {
> -	if {[lindex [file split $_gitdir] end] ne {.git}} {
> +	if {[is_config_true core.bare]} {

This doesn't work as you expect. Its a chicken-and-egg problem. We haven't read the config yet because we aren't sure that the $_gitdir really is a git directory. Consequently, core.bare is always false.

However, yes, in modern versions of git the core.bare setting in .git/config is *usually* accurate.

The only time it isn't is when its not present (really old repository which hasn't been re-init'd in a long time), or when the user does "mv .git ../foo.git" to create a bare repository from a non-bare one.

;-)
-- 
Shawn.
Giuseppe Bilotta· Feb 18, 2009, 21:47 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 1/3] git-gui: properly check for a bare repo

On Wed, Feb 18, 2009 at 6:16 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 29 quoted lines
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> wrote:
>> When bare repository handling is not enabled, check for a bare
>> repository looking at the core.bare config option rather than refusing
>> to operate with a git directory ending with .git.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>> I know I should have probably used something like
>> git rev-parse --is-bare-repository instead, but I didn't feel like
>> adding another git call. Is the config approach robust enough?
>>
>>  git-gui/git-gui.sh |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
>> index e018e07..658a728 100755
>> --- a/git-gui/git-gui.sh
>> +++ b/git-gui/git-gui.sh
>> @@ -1071,7 +1071,7 @@ if {$_prefix ne {}} {
>>       }
>>       unset cdup
>>  } elseif {![is_enabled bare]} {
>> -     if {[lindex [file split $_gitdir] end] ne {.git}} {
>> +     if {[is_config_true core.bare]} {
>
> This doesn't work as you expect.  Its a chicken-and-egg problem.
> We haven't read the config yet because we aren't sure that the
> $_gitdir really is a git directory.  Consequently, core.bare is
> always false.
Isn't the config loaded on line 1053?
Better ways to work around the issue?
-- 
Giuseppe "Oblomov" Bilotta
Shawn O. Pearce· Feb 18, 2009, 21:53 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH 1/3] git-gui: properly check for a bare repo

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> wrote:
Show 8 quoted lines
> On Wed, Feb 18, 2009 at 6:16 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> >
> > This doesn't work as you expect.  Its a chicken-and-egg problem.
> > We haven't read the config yet because we aren't sure that the
> > $_gitdir really is a git directory.  Consequently, core.bare is
> > always false.
> 
> Isn't the config loaded on line 1053?
No, that line is only run if there is no git repository here and
we are opening the repository selection wizard.  The load_config
is to pull in the user's ~/.gitconfig.
 
> Better ways to work around the issue?

Break down and run the fork command here. We already ran two git-rev-parse calls at 1049,1050. One more here during startup isn't going to kill the world.

The only problem is, you need to watch out for the git version. AFAIK git-gui still runs on git 1.5.0. Any option added since should be used only if its known to exist, or if there is a fallback that works almost as good.

Or, we need to up our minimum version around line 842, and in the messages above in 756, 787, 789, 791, and their translations...

-- 
Shawn.
Giuseppe Bilotta· Feb 9, 2009, 02:00 UTC · re: Giuseppe Bilotta · lore

[PATCH 2/3] git-gui: use the actual worktree

Don't rely on the git worktree being the updir of the gitdir, since it might not be. Instead, define (and use) a new _gitworktree global variable, setting it to $GIT_WORK_TREE if present, or to whatever we guess the correct worktree is.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-gui/git-gui.sh |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)
Show changes to git-gui/git-gui.sh +19 −6
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 658a728..94317c7 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -121,6 +121,7 @@ unset oguimsg
 
 set _appname {Git Gui}
 set _gitdir {}
+set _gitworktree {}
 set _gitexec {}
 set _reponame {}
 set _iscygwin {}
@@ -1062,13 +1063,19 @@ if {![file isdirectory $_gitdir]} {
 	error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
 	exit 1
 }
+set _gitworktree $env(GIT_WORK_TREE)
 if {$_prefix ne {}} {
-	regsub -all {[^/]+/} $_prefix ../ cdup
+	if {$_gitworktree eq {}} {
+		regsub -all {[^/]+/} $_prefix ../ cdup
+	} else {
+		set cdup $_gitworktree
+	}
 	if {[catch {cd $cdup} err]} {
 		catch {wm withdraw .}
 		error_popup [strcat [mc "Cannot move to top of working directory:"] "\n\n$err"]
 		exit 1
 	}
+	set _gitworktree [pwd]
 	unset cdup
 } elseif {![is_enabled bare]} {
 	if {[is_config_true core.bare]} {
@@ -1076,11 +1083,15 @@ if {$_prefix ne {}} {
 		error_popup [strcat [mc "Cannot use funny .git directory:"] "\n\n$_gitdir"]
 		exit 1
 	}
-	if {[catch {cd [file dirname $_gitdir]} err]} {
+	if {$_gitworktree eq {}} {
+		set _gitworktree [file dirname $_gitdir]
+	}
+	if {[catch {cd $_gitworktree} err]} {
 		catch {wm withdraw .}
-		error_popup [strcat [mc "No working directory"] " [file dirname $_gitdir]:\n\n$err"]
+		error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
 		exit 1
 	}
+	set _gitworktree [pwd]
 }
 set _reponame [file split [file normalize $_gitdir]]
 if {[lindex $_reponame end] eq {.git}} {
@@ -1878,6 +1889,7 @@ proc incr_font_size {font {amt 1}} {
 set starting_gitk_msg [mc "Starting gitk... please wait..."]
 
 proc do_gitk {revs} {
+	global _gitworktree
 	# -- Always start gitk through whatever we were loaded with.  This
 	#    lets us bypass using shell process on Windows systems.
 	#
@@ -1895,7 +1907,7 @@ proc do_gitk {revs} {
 		}
 
 		set pwd [pwd]
-		cd [file dirname [gitdir]]
+		cd $_gitworktree
 		set env(GIT_DIR) [file tail [gitdir]]
 
 		eval exec $cmd $revs &
@@ -1915,6 +1927,7 @@ proc do_gitk {revs} {
 }
 
 proc do_explore {} {
+	global _gitworktree
 	set explorer {}
 	if {[is_Cygwin] || [is_Windows]} {
 		set explorer "explorer.exe"
@@ -1924,7 +1937,7 @@ proc do_explore {} {
 		# freedesktop.org-conforming system is our best shot
 		set explorer "xdg-open"
 	}
-	eval exec $explorer [file dirname [gitdir]] &
+	eval exec $explorer $_gitworktree &
 }
 
 set is_quitting 0
@@ -3302,7 +3315,7 @@ unset i
 set file_lists($ui_index) [list]
 set file_lists($ui_workdir) [list]
 
-wm title . "[appname] ([reponame]) [file normalize [file dirname [gitdir]]]"
+wm title . "[appname] ([reponame]) [file normalize $_gitworktree]"
 focus -force $ui_comm
 
 # -- Warn the user about environmental problems.  Cygwin's Tcl

-- 
1.6.2.rc0.173.g5e148
Shawn O. Pearce· Feb 18, 2009, 17:22 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH 2/3] git-gui: use the actual worktree

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> wrote:
Show 19 quoted lines
> Don't rely on the git worktree being the updir of the gitdir, since it
> might not be. Instead, define (and use) a new _gitworktree global
> variable, setting it to $GIT_WORK_TREE if present, or to whatever we
> guess the correct worktree is.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  git-gui/git-gui.sh |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 658a728..94317c7 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1062,13 +1063,19 @@ if {![file isdirectory $_gitdir]} {
>  	error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
>  	exit 1
>  }
> +set _gitworktree $env(GIT_WORK_TREE)

In TCL it is an error if an environment variable is undefined when accessed. You need to wrap this up in a catch block to handle the error:

Error in startup script: can't read "env(GIT_WORK_TREE)": no such variable
    while executing
"set _gitworktree $env(GIT_WORK_TREE)"
    (file "./git-gui.sh" line 1066)

Also, what about honoring core.worktree as a setting, in addition to $env(GIT_WORK_TREE) like the git wrapper does?

-- 
Shawn.
Giuseppe Bilotta· Feb 9, 2009, 02:00 UTC · re: Giuseppe Bilotta · lore

[PATCH 3/3] git-gui: define correct GIT_DIR for gitk

When invoking gitk, git-gui assumed that the worktree was the dirname of $_gitdir and that, by consequence, the git dir could be set to the tail of $_gitdir once we changed to the worktree root directory.

The assumption is no longer true since the previous commit introduced support for more generic worktree locations, so export a GIT_DIR environment variable set to the full, normalized path of $_gitdir instead.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 git-gui/git-gui.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-gui/git-gui.sh +1 −1
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 94317c7..de3f29c 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1908,7 +1908,7 @@ proc do_gitk {revs} {
 
 		set pwd [pwd]
 		cd $_gitworktree
-		set env(GIT_DIR) [file tail [gitdir]]
+		set env(GIT_DIR) [file normalize [gitdir]]
 
 		eval exec $cmd $revs &
 

-- 
1.6.2.rc0.173.g5e148
Shawn O. Pearce· Feb 18, 2009, 17:22 UTC · re: Giuseppe Bilotta · lore

Re: [PATCH 3/3] git-gui: define correct GIT_DIR for gitk

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> wrote:
Show 8 quoted lines
> When invoking gitk, git-gui assumed that the worktree was the dirname of
> $_gitdir and that, by consequence, the git dir could be set to the tail of
> $_gitdir once we changed to the worktree root directory.
> 
> The assumption is no longer true since the previous commit introduced
> support for more generic worktree locations, so export a GIT_DIR
> environment variable set to the full, normalized path of $_gitdir
> instead.
Since this is a one line patch to fix a bug introduced by the prior
patch, I'd rather just see this squash into the prior patch.
Show 21 quoted lines
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  git-gui/git-gui.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 94317c7..de3f29c 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1908,7 +1908,7 @@ proc do_gitk {revs} {
>  
>  		set pwd [pwd]
>  		cd $_gitworktree
> -		set env(GIT_DIR) [file tail [gitdir]]
> +		set env(GIT_DIR) [file normalize [gitdir]]
>  
>  		eval exec $cmd $revs &
>  
> -- 
> 1.6.2.rc0.173.g5e148
> 
-- 
Shawn.

← back to recent threads