Re: [PATCH v3 1/1] git-gui: handle missing worktree and separated gitdir

[Mark Levedahl <mlevedahl@gmail.com>]

On 5/1/26 12:42 PM, Mark Levedahl wrote:
> On 5/1/26 9:13 AM, Johannes Sixt wrote:
>> Am 30.04.26 um 18:18 schrieb Mark Levedahl:
>>> On 4/30/26 6:02 AM, Shroom Moo wrote:
>>> We have quite a bit of code that attempts to make Git GUI work from the
>> .git directory and also in bare repositories.
>>
>> 87cd09f43e56 ("git-gui: work from the .git dir", 2010-01-23) made the
>> first step. The original code just used the $_gitdir as the working
>> directory. However, at that time we did not have alternate worktrees,
>> and the old code, when used today, does not work in a `git
>> worktree`-created worktree. Later, the `git rev-parse --show-toplevel`
>> call came with 38ec8d3e2652 ("git-gui: correct assignment of work-tree",
>> 2010-10-20). However, it also changes the fall-back code slightly, so
>> that running Git GUI from the .git directory would not work the same way
>> as before and takes the .git directory as the work tree (because in the
>> .git directory --show-cdup is not "..", but empty).
>>
>> I think we need to restructure the existing flow a bit and not just fix
>> a single spot in the code. I suggest this order of operation:
>>
>> 1. Handle the bare repository case. If not enabled, fail. Otherwise, we
>> can work with an empty $_gitworktree.
>>
>> 2. Collect --show-toplevel into $_gitworktree.
>>
>> 2a. If this failed: If --is-inside-git-dir is true, and the last
>> $_gitdir directory component is exactly ".git", take the parent
>> repository as $_gitworktree. Otherwise, fail.
>>
>> 3. Handle all the other edge cases, if any, with the so determined
>> $_gitworktree. (I didn't think through, yet, what needs to be done.)
>>
>> -- Hannes
>>
> I found one horrid edge case: 
>
> Start git-gui in a gitdir not embedded in a worktree, with core.bare=false as there are
> one or more gitfile and/or symlinked worktrees elsewhere.
> - current git-gui aborts with an uncaught error. Good.
> - git-gui with the wrapped --show-toplevel call finds no worktree to switch to, so runs in
> the gitdir allowing commits of the gitdir items.
> -  I just added and  committed the *file* refs/heads/master to branch master in such a gitdir.
>
> git-gui's normal gui must be started ONLY if rev-parse --is-inside-work-tree is true. (The
> blame view invoked by gitk in theory could be allowed to run in a bare repository
> read-only mode.).
>
> For read/write mode:
> if --is-inside-git-dir == true at startup, we must abort, or find a valid worktree and
> switch to that.
>
> My personal preference is for git-gui to abort:
>    I started git-gui where it cannot run. 
>    My error. Let me learn and fix that.
>
> Alternatively, ask me what to do:
>     e.g., prepare a dialog after looking at git-worktree list, and the parent dir IFF this
> dir is named .git, telling me of my mistake and offering me one or more worktrees to
> switch to.
>
> But please, don't just switch to another directory without asking. This is just
> encouraging me to make careless errors.
>
> Mark
>
I dug a bit more into the startup logic, and I think I better understand rework that is
needed.

Two basic problems I see here are beyond the question of if (and when) git-gui should try
to locate a worktree:

- git gui blame in a gitdir was apparently broken by the git repo commit 2d92ab32fd
("rev-parse: make --show-toplevel without a worktree an error", 2019-11-19). Prior to that
commit, git gui would stay in the startup directory enabling only features that cannot
modify the repository, and gitk could bring this view up in a gitdir. This doesn't work
right now.

- git-gui's logic includes a conceptual error embodied in proc is_bare: is_bare uses $(git
rev-parse --is-bare-repository) but what we need is $(git rev-parse --is-inside-git-dir),
and these are not synonyms.
It does not matter whether a worktree exists that points at the gitdir, and as discussed
before, main worktrees can easily exist that we cannot locate from the gitdir. At best,
is_bare is a guess.
So, is_bare should be replaced by is_inside_gitdir, and we should also have
is_inside_worktree. These are mutually exclusive, though both can be false.

My current idea of an improved startup flow enables features only at the end:

1) If not in a gitdir or worktree, 
    1a)   if git gui's subcommand is not 'gui', abort with an error (citool, browser, or
blame invocations carry information specific to a gitdir/worktree). 
    1b)   otherwise, invoke repository_chooser, which either aborts, or changes directory
to a worktree.

-- we are now in a gitdir or a worktree, and this may or may not be the startup directory.

2) Look at the combination of git gui subcommand and directory type (worktree / gitdir) to
decide to continue.
    2a) blame / browser are ok in either directory type.
    2b) citool requires a specific worktree, which should have been the initial startup
directory. Abort if not.
    2b) gui requires a worktree. Abort if not (my recommendation), or offer to find (or
automatically find) a worktree.

3) Change directory to the top level of of the directory_type (git rev-parse knows
toplevel of a worktree, different code is needed for a gitdir).

4) Enable features based upon subcommand and directory type.

There are 12 combinations of initial directory type (gitdir, worktree, neither) and
subcommand (gui, blame, browser, citool) to consider, with a lot of duplicated code
amongst the 12 cases. So, obviously, steps 1 and 2 can be convolved in many ways that are
different than what I wrote above.

Mark