On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@xxxxxxxxx> writes: > > > /* > > - * TODO: heed core.bare from config file in templates if no > > - * command-line override given > > + * Note: The below line simply checks the presence of worktree (the > > + * simplification of which is given after the line) and core.bare from > > + * config file is not taken into account when deciding if the worktree > > + * should be created or not, even if no command line override given. > > + * That is intentional. Therefore, if in future we want to heed > > + * core.bare from config file, we should do it before we create any > > + * subsequent directories for worktree or repo because until this point > > + * they should already be created. > > */ > > is_bare_repository_cfg = prev_bare_repository || !work_tree; > > I do not recall the discussion; others may want to discuss if the > change above is desirable, before I come back to the topic later. > > But I see this long comment totally unnecessary and distracting. > > > - /* TODO (continued): > > + /* Note (continued): > > * > > - * Unfortunately, the line above is equivalent to > > + * The line above is equivalent to > > * is_bare_repository_cfg = !work_tree; > > - * which ignores the config entirely even if no `--[no-]bare` > > - * command line option was present. > > * > > * To see why, note that before this function, there was this call: > > * prev_bare_repository = is_bare_repository() > > If it can be proven that the assignment can be simplified to lose > the "prev_bare_repository ||" part, then the above comment can be > used as part of the proposed log message for a commit that makes > such a change. There is no reason to leave such a long comment to > leave the more complex "A || B" expression when it can be simplified > to "B", no? I agree. In fact, we can remove the prev_bare_repository variable altogether as it was used solely for the "A || B" expression. Initially this function used to be in builtin/init-db.c and shared with builtin/clone.c. In moving to setup.c, an unnecessary global variable equivalent to prev_bare_repository was removed and therefore prev_bare_repository was intruduced to not hinder the future possibility of intruducing the (core.bare from config) feature which might have been the global variables partial intent[1]. Therefore, I kept the original expression. However, in the same series that this was introduced, it was acknowledged by Elijah[2] that create_default_files() would possibly be too late to heed core.bare. And indeed it is, as the directories for the worktree or repo are already created by this point. Therefore, prev_bare_repository does not seem to have any usecase with/without supporting core.bare from config. [1]: https://lore.kernel.org/git/xmqqsf1bf5ew.fsf@gitster.g/T/#m64125dd80d04ae177944434e7092522325b374c9 [2]: 0f7443bdc7 (init-db: document existing bug with core.bare in template config, 2023-05-16) Thanks.
