| Message ID | 20240229134114.285393-2-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | setup: clarify TODO comment about ignoring core.bare | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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? Thanks.
On Fri Mar 1, 2024 at 12:45 AM IST, Junio C Hamano wrote: > Ghanshyam Thakkar <shyamthakkar001@gmail.com> 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.
diff --git a/setup.c b/setup.c index b69b1cbc2a..92e0c3a121 100644 --- a/setup.c +++ b/setup.c @@ -1997,16 +1997,20 @@ static int create_default_files(const char *template_path, if (init_shared_repository != -1) set_shared_repository(init_shared_repository); /* - * 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; - /* 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() diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index b1eb5c01b8..29cf8a9661 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -52,7 +52,7 @@ test_expect_success 'shared=all' ' test 2 = $(git config core.sharedrepository) ' -test_expect_failure 'template can set core.bare' ' +test_expect_success 'template cannot set core.bare' ' test_when_finished "rm -rf subdir" && test_when_finished "rm -rf templates" && test_config core.bare true && @@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' ' mkdir -p templates/ && cp .git/config templates/config && git init --template=templates subdir && - test_path_exists subdir/HEAD -' - -test_expect_success 'template can set core.bare but overridden by command line' ' - test_when_finished "rm -rf subdir" && - test_when_finished "rm -rf templates" && - test_config core.bare true && - umask 0022 && - mkdir -p templates/ && - cp .git/config templates/config && - git init --no-bare --template=templates subdir && - test_path_exists subdir/.git/HEAD + test_path_is_missing subdir/HEAD ' test_expect_success POSIXPERM 'update-server-info honors core.sharedRepository' ' diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index a400bcca62..e93e0d0cc3 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' ' ' -test_expect_failure 'prefers --template config even for core.bare' ' +test_expect_success 'ignore --template config for core.bare' ' template="$TRASH_DIRECTORY/template-with-bare-config" && mkdir "$template" && git config --file "$template/config" core.bare true && git clone "--template=$template" parent clone-bare-config && - test "$(git -C clone-bare-config config --local core.bare)" = "true" && - test_path_is_file clone-bare-config/HEAD + test "$(git -C clone-bare-config config --local core.bare)" = "false" && + test_path_is_missing clone-bare-config/HEAD ' test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
The comment suggested to heed core.bare from template config if no command line override given. However, it was clarified by Junio that such values are ignored by intention and does not make sense to propagate it from template config to the repository config[1]. Therefore, remove the TODO tag and update the comment to add additional context if such functionality is desired in the future. Also update the relevant test cases to not expect failure and remove one redundant testcase. [1]: https://lore.kernel.org/git/xmqqjzonpy9l.fsf@gitster.g/ Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> --- setup.c | 16 ++++++++++------ t/t1301-shared-repo.sh | 15 ++------------- t/t5606-clone-options.sh | 6 +++--- 3 files changed, 15 insertions(+), 22 deletions(-)