On Sun, Dec 19, 2021 at 6:54 PM <rsbecker@xxxxxxxxxxxxx> wrote: > On December 19, 2021 6:30 PM, Eric Sunshine wrote: > > `is_bare_repository_cfg` is not exactly a boolean; it's a tristate, with -1 > > meaning "not yet determined". I didn't, at the time, closely follow the > > discussion[1] of the particular bit of code you're questioning, but the `== 1` > > was mentioned at least a couple times, once in review by Junio[2], and then > > in the extra patch commentary by "jtan" when he submitted v2[3]. Anyhow, > > if I'm following the original discussion correctly, then the usage, `== 1` (or the > > equivalent `> 0`) is probably correct, and that treating it as a simple boolean > > (where > > -1 is true, too) would be undesirable. (Of course, I haven't traced through the > > init code at all, so I don't even know if it can ever be > > -1 at this point.) Five existing consumers of this global variable use `== 1`, and > > only two use `> 0`, so this usage is at least reasonably consistent with other > > parts of the project. > > Thanks for the clarification. It helps to understand the code. Could the variable type be changed to a new typedef like ConfigTriState instead of int to be clear about its semantics? Or perhaps an enum with -1, 0, 1 declared explicitly? I'm not sure how much value an enum would add in this particular case since this is a quite common idiom in this codebase. It might be sufficient merely to add a comment to its declaration in cache.h explaining its possible values. What could be even more helpful is some documentation about the entire setup sequence and how all the different "is bare" and "do we have a worktree" computations interact since the general setup logic is quite involved, and there are likely a number of pitfalls which are not obvious by merely glancing at the code, but which people know about who have dealt extensively with this code (such as Duy and probably Peff).
