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

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