Re: [PATCHv2 1/2] git-gui: handle non-standard worktree locations

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 19, 2009, 02:22 UTC
Message-ID: <alpine.DEB.1.00.0902190307390.10279@pacific.mpi-cbg.de>
In-Reply-To: <1235006145-16760-2-git-send-email-giuseppe.bilotta@gmail.com>

Hi,

whenever I read "worktree" I get cautious.

_Very_ cautious.

I mean, _VERY_ cautious.  And for a reason.

The whole idea of a worktree was bolted onto Git, so much so that almost invariably, every attempt at making it work properly, fails.

Honestly, I think it caused us more pain than any other part of Git, _including_ the nasty bugs in git_config_set().

On Thu, 19 Feb 2009, Giuseppe Bilotta wrote:

> Don't

Personally, I prefer a less colloquial (and while at it, a more concise) commit message.

Show 14 quoted lines

> +# _gitdir exists, so try loading the config
> +load_config 0
> +apply_config
> +# try to set work tree from environment, falling back to core.worktree
> +if {[catch { set _gitworktree $env(GIT_WORK_TREE) }]} {
> +	set _gitworktree [get_config core.worktree]
> +}
>  if {$_prefix ne {}} {
> -	regsub -all {[^/]+/} $_prefix ../ cdup
> +	if {$_gitworktree eq {}} {
> +		regsub -all {[^/]+/} $_prefix ../ cdup
> +	} else {
> +		set cdup $_gitworktree
> +	}

It appears as if you redo a the logic laid out in setup.c. Don't. Instead, teach rev-parse to output the path of the work tree.

Oh, wait, --show-cdup already shows that...

Show 8 quoted lines

> @@ -1076,11 +1089,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]
> +	}

This is certainly wrong in bare repositories.

(If that code is only exercized in non-bare repositories, you failed to indicate that. The commit message is the place for such comments.)

Show 7 quoted lines

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

And why do you have to cd to the worktree? You might not be able to come back to the GIT_DIR, after all.

Show 13 quoted lines

> @@ -1895,8 +1913,10 @@ proc do_gitk {revs} {
>  		}
>  
>  		set pwd [pwd]
> -		cd [file dirname [gitdir]]
> -		set env(GIT_DIR) [file tail [gitdir]]
> +		if { $_gitworktree ne {} } {
> +			cd $_gitworktree
> +		}
> +		set env(GIT_DIR) [file normalize [gitdir]]
>  
>  		eval exec $cmd $revs &
>

Just out of curiosity: what was your method of making sure that you catch all sites where the worktree is assumed to be $GIT_DIR/..?

That is a most crucial information for the commit message!!!

Besides, have you read about the fallout of making "git rev-parse --git-dir" showing absolute paths always?

If yes, why do you ignore it in your patch?

Ciao, Dscho