Thanks for the response. I realized where my misunderstanding was. Once again reordering slightly.. Elijah Newren <newren@xxxxxxxxx> writes: >> Where I struggling with is how to make this behave badly. The lines >> above seem to be defensive in nature - we never use >> is_bare_repository_cfg past this point, but we want to guard against >> unintentional behavior in the future. > > Not quite true; there is another call to is_bare_repository() in the > same function about 60 lines later, which does pay attention to > is_bare_repository_cfg. Ah thanks. I didn't realize that the "is_bare_repository_cfg" that holds the value of the CLI option is actually the global variable that also stores cached value of "core.bare" (I thought it was a local variable). This may not be in scope for your series, but if we want to preserve the CLI option like we say we do... >> Now, we're in the midst of the re-init. Expanding the context a little, >> we see: >> >> git_config(git_default_config, NULL); >> >> /* >> * We must make sure command-line options continue to override any >> * values we might have just re-read from the config. >> */ >> is_bare_repository_cfg = init_is_bare_repository || !work_tree; >> >> So now we've read the new config of the re-inited repo, which might have >> "core.bare" set to a value other than what "git init-db [--bare]" >> started with > > Correct. > >> so we want to _intentionally_ ignore it. > > That's what the code does, but not what it should do. If neither > `--bare` nor `--no-bare` is given on the command line, we ought to pay > attention to the config setting. > > The fact that we do ignore the config regardless of command line flags > is the bug that this patch documents. wouldn't be a lot easier to parse the option into a local variable instead of reusing the config one? Then we always have the original CLI value available to us and can restore it back to "is_bare_repository_cfg" if we really wanted to. I'm not sure if this means we can/should drop the "|| !work_tree" in the hunk above. It would nice if we could, I find it very confusing. > So, both before and after what we are computing simplifies to > <something> && !work_tree || !work_tree > which further simplifies, regardless of whether <something> is true or > false, down to > !work_tree > > So whether we are using a cached value of is_bare_repository_cfg or a > current value of is_bare_repository_cfg is irrelevant. In fact, from > the analysis above, we can simplify this line to just > is_bare_repository_cfg = !work_tree; > completely ignoring both is_bare_repository_cfg and > is_bare_repository(), and it won't change the behavior. I just did > it; it passes all tests. Hm.. that doesn't sound intended, and fixing this is probably isn't in scope for this series either. >> If I'm right (which I'm not sure about), we might need to keep >> init_is_bare_repository around _somewhere_. Not a global, but maybe >> as a param. > > This makes sense, despite the other bugs present. I'll make this > change, as well as split this patch into two as Calvin suggested, and > update the description to correct my errors and explain the bug > better. Yeah, I think this might be the smallest obviously correct thing. We can clean up all of the other bits outside of a big refactor series.
