Re: [PATCH v1 09/11] git-gui: support using repository parent dir as a worktree

[Johannes Sixt <j6t@kdbg.org>]

Am 14.05.26 um 16:33 schrieb Mark Levedahl:
> git-gui, since 87cd09f43e ("git-gui: work from the .git dir",
> 2010-01-23), has had the intent to allow starting from inside a
> repository, then switching to the parent directory if that is a valid
> worktree.
> 
> This certainly hasn't worked since 2d92ab32fd ("rev-parse: make
> --show-toplevel without a worktree an error", 2019-11-19) in git, but
> breaking this git-gui feature was unintentional.
> 
> Add a proc to test if the parent of the git repository is a valid
> worktree, and set that directory as the worktree if so. Use invocations
> of git rev-parse to assure all validity and safety checks included in
> git-core are executed.

BTW, missing sign-off.

> ---
>  git-gui.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index a03eaa7..e326401 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1100,6 +1100,23 @@ unset argv0dir
>  ##
>  ## repository setup
>  
> +proc is_parent_worktree {} {
> +	# Directory 'parent' of a repository named 'parent/.git' might be the worktree
> +	set ok 0
> +	if {[file tail $::_gitdir] eq {.git}} {
> +		set gitdir_parent [file join $::_gitdir {..}]
> +		set expected_worktree [file normalize $gitdir_parent]

We have [file dirname ...]. Is there a reason to avoid it?

> +		catch {set git_worktree [git -C $gitdir_parent rev-parse --show-toplevel]}
> +		if {[string compare $expected_worktree $git_worktree] == 0} {

The purpose of this check should be explained in a comment. I think it is:

For a repository with the database in a directory named .git we assume
that the working tree is the directory containing .git. But
configuration may point to a different worktree. Then we do not want to
hold on to our assumption.

However, whether [git -C elsewhere ...] uses the same gitdir that we
have discovered so far cannot be told from this piece of code alone.
Therefore, I think it is wrong to extract this check into a function.

Also, I don't think we can use string comparison here. On Windows, the
command returns the Windows style path, but Tcl my operate with a POSIX
style path.

> +			set ::_prefix {}
> +			set ::_gitworktree $git_worktree
> +			cd $git_worktree

So many side-effects in a function whose name suggests that it only does
some checks. Please, don't do that.

> +			set ok 1
> +		}
> +	}
> +	return $ok
> +}
> +
>  proc is_gitvars_error {err} {
>  	set havevars 0
>  	set GIT_DIR {}

In general, I am not a fan of commits that add new functions, but no
call sites. Please squash this into 10/11. Ditto for is_gitvars_error in
06/11.

-- Hannes