Re: [PATCH v5 1/1] git-gui: restructure repository startup
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- May 6, 2026, 07:15 UTC
- Message-ID
- <28db8297-c995-4e29-ae44-ade304b2aad5@kdbg.org>
- In-Reply-To
- <tencent_78B80FB7A0A42E464B3EF1841E2AF3C39509@qq.com>
Am 04.05.26 um 16:59 schrieb Shroom Moo:
Show 23 quoted lines
> When git-gui is started inside a .git directory of a non-bare > repository, it should treat the parent directory as the worktree, > as it did before commit 2d92ab32fd (rev-parse: make --show-toplevel > without a worktree an error, 2019-11-19). However, a bare repository > or a separated gitdir without a worktree must be rejected early. > > Protect the previously unguarded calls to `git rev-parse > --show-object-format` and `--show-toplevel`. Restructure the startup > sequence to: > > - Check for a bare repository right after loading the config. If the > repository is bare and the current subcommand does not allow bare > repos (e.g. normal commit mode), show "Cannot use bare repository" > and exit. > > - When `rev-parse --show-toplevel` fails and the repository is > non-bare, the gitdir path ends with ".git", and we are inside that > gitdir, use the parent directory as the worktree. This preserves > the ability to start git-gui from within a regular repository’s > .git directory, which was intentionally supported since 87cd09f43e56 > (git-gui: work from the .git dir, 2010-01-23). > > - Otherwise, show a descriptive error and exit.
Very good. This does things in the right order, IMO.
> > - Wrap `rev-parse --show-object-format` in a catch to avoid a crash > when the repository configuration is broken (e.g. core.worktree > pointing to an invalid path).
Nice catch. This could be moved into its own patch. But it is acceptable in this patch as it loosely fits the topic.
Show 9 quoted lines
> > Also removes the old `_prefix`‑based fallback that computed a relative > path to the worktree top from a subdirectory, and the unconditional > `[file dirname $_gitdir]` guess. Both are unnecessary now that > `rev‑parse --show‑toplevel` directly provides the absolute top‑level > path and we can `cd` to it. The guess is further unsafe in > multi‑worktree setups, where a gitdir may have more than one worktree. > The only remaining fallback is the explicit “.git directory” rule for > non‑bare repositories, which mirrors the historical behaviour.
Nice cleanup.
> > This fixes the fatal Tcl error when the working tree is missing, while > keeping the .git startup feature and avoiding any automatic directory > switching that could be dangerous in multi‑worktree setups.
Good!
However, this doesn't fix `git gui blame HEAD file` in a bare repository, because `git branch --show-current` fails with an empty GIT_WORK_TREE value. Fixing this needs to consider whether to set GIT_DIR and GIT_WORK_TREE at all, as alluded to by Mark in a near-by message. This is a separate topic.
Show 76 quoted lines
>
> Signed-off-by: Shroom Moo <egg_mushroomcow@foxmail.com>
> ---
> git-gui/git-gui.sh | 72 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 45 insertions(+), 27 deletions(-)
>
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 23fe76e498..c06d85b8d9 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1129,7 +1129,8 @@ if {[catch {
> }]
> && [catch {
> # beware that from the .git dir this sets _gitdir to .
> - # and _prefix to the empty string
> + # and _prefix to the empty string; this is handled by
> + # the startup safety checks below
> set _gitdir [git rev-parse --git-dir]
> set _prefix [git rev-parse --show-prefix]
> } err]} {
> @@ -1142,8 +1143,20 @@ if {[catch {
> set picked 1
> }
>
> +if {![file isdirectory $_gitdir]} {
> + catch {wm withdraw .}
> + error_popup [strcat
> + [mc "Git directory not found:"] "\n\n$_gitdir\n\n" \
> + [mc "Please ensure GIT_DIR points to a valid Git repository"]]
> + exit 1
> +}
> +
> # Use object format as hash algorithm (either "sha1" or "sha256")
> -set hashalgorithm [git rev-parse --show-object-format]
> +if {[catch {set hashalgorithm [git rev-parse --show-object-format]} err]} {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Failed to determine hash algorithm:"] "\n\n$err"]
> + exit 1
> +}
> if {$hashalgorithm eq "sha1"} {
> set hashlength 40
> } elseif {$hashalgorithm eq "sha256"} {
> @@ -1160,46 +1173,50 @@ if {$_gitdir eq "."} {
> set _gitdir [pwd]
> }
>
> -if {![file isdirectory $_gitdir]} {
> - catch {wm withdraw .}
> - error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
> - exit 1
> -}
> # _gitdir exists, so try loading the config
> load_config 0
> apply_config
>
> -set _gitworktree [git rev-parse --show-toplevel]
> +# Handle bare repository early: if not allowed, abort
> +if {[is_bare] && ![is_enabled bare]} {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" [file normalize $_gitdir]]
> + exit 1
> +}
>
> -if {$_prefix ne {}} {
> - 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
> +# Determine the working tree
> +if {[is_bare] && [is_enabled bare]} {
> + set _gitworktree {}I strongly suggest to collapse this branch and the previous 'if {[is_bare] && ![is_enabled bare]}' into a single 'if {[is_bare]}', because then in the else-branch below we can be sure not to have a bare repository.
Show 7 quoted lines
> +} else {
> + if {[catch {set _gitworktree [git rev-parse --show-toplevel]} err]} {
> + # If we are inside a .git directory of a non-bare repo,
> + # the worktree is the parent directory
> + set inside_gitdir 0
> + catch {set inside_gitdir [git rev-parse --is-inside-git-dir]}
> + if {![is_bare] && $inside_gitdir eq {true} && [file tail [file normalize $_gitdir]] eq {.git}} {Do we need to 'file normalize' before taking the 'file tail'? If not, then the line would be shorter.
Show 43 quoted lines
> + set _gitworktree [file normalize [file dirname $_gitdir]]
> + } else {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Cannot determine working tree:"] "\n\n$err"]
> + exit 1
> + }
> }
> - set _gitworktree [pwd]
> - unset cdup
> -} elseif {![is_enabled bare]} {
> - if {[is_bare]} {
> +
> + if {$_gitworktree eq {}} {
> catch {wm withdraw .}
> - error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"]
> + error_popup [mc "Cannot determine working tree (unexpected empty result)"]
> exit 1
> }
> - if {$_gitworktree eq {}} {
> - set _gitworktree [file dirname $_gitdir]
> - }
> +
> if {[catch {cd $_gitworktree} err]} {
> catch {wm withdraw .}
> - error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
> + error_popup [strcat [mc "Cannot move to working directory:"] "\n\n$err"]
> exit 1
> }
> set _gitworktree [pwd]
> }
> +
> +# Derive a human-readable repository name
> set _reponame [file split [file normalize $_gitdir]]
> if {[lindex $_reponame end] eq {.git}} {
> set _reponame [lindex $_reponame end-1]
> @@ -1207,6 +1224,7 @@ if {[lindex $_reponame end] eq {.git}} {
> set _reponame [lindex $_reponame end]
> }
>
> +# Export the final paths
> set env(GIT_DIR) $_gitdir
> set env(GIT_WORK_TREE) $_gitworktree
> -- Hannes