On Fri, Apr 19, 2019 at 1:30 AM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: > > > You actually didn't spell out the problem with "git branch -D", or at > > least the consequence (i.e. the submodule branch is deleted even if > > it's checked out). > > Thanks - I'll do that in the commit message. Another minor nit (because I was still puzzled why a submodule was considered bare/not bare) > This is because get_main_worktree() in worktree.c sets is_bare on a > worktree only using the heuristic that a repo is bare if the worktree's > path ends in "/.git", and not bare otherwise. s/ends/does not end/. Now it makes more sense because the submodule's main worktree is accidentally considered "bare" (i.e. no worktree), its HEAD is ignored. > > > strbuf_addf(&path, "%s/HEAD", get_git_common_dir()); > > > > > > worktree = xcalloc(1, sizeof(*worktree)); > > > worktree->path = strbuf_detach(&worktree_path, NULL); > > > - worktree->is_bare = is_bare; > > > + worktree->is_bare = (is_bare_repository_cfg == 1) || > > > > core.bare and core.worktree are special. When you access them standing > > from the main worktree, you'll see them. But when you stand from a > > secondary worktree, they are ignored. > > Just checking: I think that is_bare_repository_cfg ignores core.bare > only if the config.worktree file is present? No, both are ignored independently if you read them from a secondary worktree. Tested too with rev-parse, just to be sure. > In the t2402 test '"list" > all worktrees from linked with a bare main', "git worktree list" still > observes the main worktree as bare. Yes, because of bug :( When git_config() is called again by cmd_worktree(), it does not treat core.worktree and core.bare special anymore. So is_bare_repository_cfg is reset from 0 to 1, even though I think its value at that point plays no role anymore. What matters is the value at setup_git_directory(). So yeah your code works in all cases by luck ;) Fixing that one will not be easy (to avoid traps). I did try to delete is_bare_repository_cfg (on the ground that global vars are hard to manage, as seen here) only to discover that I could not simply delete the core.bare parsing code in git_default_core_config() without changing behaviour in some case [1]. I should probably revive that branch (cleaning up gitdir setup code a bit) and submit. [1] https://gitlab.com/pclouds/git/commit/2cc46d698ebe7af295e0d91f68103675077df68e#db04685516ffb491eb4239291b892fc0784e1aea > > I don't think multiple-worktrees-on-submodules will be coming soon, so > > it's probably ok. But maybe leave a note here. > > Observing that the problematic case is the 3rd one above, would this > note work: > > NEEDSWORK: If this function is called from a secondary worktree and > config.worktree is present, is_bare_repository_cfg will reflect the > contents of config.worktree, not the contents of the main worktree. > This means that worktree->is_bare may be set to 0 even if the main > worktree is configured to be bare. Even though your code works perfectly now, I still think it's good to have some comment like this. -- Duy
