On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote:

Ghanshyam Thakkar [off-list ref] writes:

quoted

 	/*
-	 * 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.

quoted

-	/* 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 (local)
[2]: 0f7443bdc7 (init-db: document existing bug with core.bare in
template config, 2023-05-16)

Thanks.