{"thread":{"id":"59868","subject":"[PATCH] setup.c: don't setup in discover_git_directory()","startedAt":"2023-06-14T19:36:05Z","lastAt":"2023-06-16T16:03:51Z","messageCount":3,"participants":["Glen Choo via GitGitGadget","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478395","messageId":"pull.1526.git.git.1686771358484.gitgitgadget@gmail.com","threadId":"59868","inReplyTo":null,"subject":"[PATCH] setup.c: don't setup in discover_git_directory()","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-14T19:35:58Z","receivedAt":"2023-06-14T19:36:05Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo \n\ndiscover_git_directory() started modifying the_repository in ebaf3bcf1ae\n(repository: move global r_f_p_c to repo struct, 2021-06-17), when, in\nthe repository setup process, we started copying members from the\n\"struct repository_format\" we're inspecting to the appropriate \"struct\nrepository\". However, discover_git_directory() isn't actually used in\nthe setup process (its only caller in the Git binary is\nread_early_config()), so it shouldn't be doing this setup at all!\n\nAs explained by 16ac8b8db6 (setup: introduce the\ndiscover_git_directory() function, 2017-03-13) and the comment on its\ndeclaration, discover_git_directory() is intended to be an entrypoint\ninto setup.c machinery that allows the Git directory to be discovered\nwithout side effects, e.g. so that read_early_config() can read\n\".git/config\" before the_repository has been set up.\n\nFortunately, we didn't start to rely on this unintended behavior between\nthen and now, so we let's just remove it. It isn't harming anyone, but\nit's confusing.\n\nSigned-off-by: Glen Choo \n---\n setup.c: don't setup in discover_git_directory()\n \n This is the scissors patch I sent on Victoria's series [1], but rebased\n onto \"master\" since that series hasn't been merged yet. The merge\n conflict resolution is to delete all of the conflicting lines:\n \n -\tthe_repository->repository_format_worktree_config =\n -\t\tcandidate.worktree_config;\n -\n \n \n IOW it's the original scissors patch if queued on top of Victoria's\n series, but it might be cleaner to invert that, i.e. if we pretended\n that this was in \"master\" already, there wouldn't be reason to add those\n lines to begin with.\n \n [1]\n https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1526%2Fchooglen%2Fpush-nknkwmnkxolv-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1526/chooglen/push-nknkwmnkxolv-v1\nPull-Request: https://github.com/git/git/pull/1526\n\n setup.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 458582207ea..bbd95f52c0f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1423,11 +1423,6 @@ int discover_git_directory(struct strbuf *commondir,\n \t\treturn -1;\n \t}\n \n-\t/* take ownership of candidate.partial_clone */\n-\tthe_repository->repository_format_partial_clone =\n-\t\tcandidate.partial_clone;\n-\tcandidate.partial_clone = NULL;\n-\n \tclear_repository_format(&candidate);\n \treturn 0;\n }\n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478467","messageId":"9a7602ba-6903-a94a-3bb5-e51c76f08058@gmx.de","threadId":"59868","inReplyTo":"pull.1526.git.git.1686771358484.gitgitgadget@gmail.com","subject":"Re: [PATCH] setup.c: don't setup in discover_git_directory()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-06-16T11:50:22Z","receivedAt":"2023-06-16T11:50:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Glen,\n\nOn Wed, 14 Jun 2023, Glen Choo via GitGitGadget wrote:\n\n> From: Glen Choo \n>\n> discover_git_directory() started modifying the_repository in ebaf3bcf1ae\n> (repository: move global r_f_p_c to repo struct, 2021-06-17), when, in\n> the repository setup process, we started copying members from the\n> \"struct repository_format\" we're inspecting to the appropriate \"struct\n> repository\". However, discover_git_directory() isn't actually used in\n> the setup process (its only caller in the Git binary is\n> read_early_config()), so it shouldn't be doing this setup at all!\n>\n> As explained by 16ac8b8db6 (setup: introduce the\n> discover_git_directory() function, 2017-03-13) and the comment on its\n> declaration, discover_git_directory() is intended to be an entrypoint\n> into setup.c machinery that allows the Git directory to be discovered\n> without side effects, e.g. so that read_early_config() can read\n> \".git/config\" before the_repository has been set up.\n>\n> Fortunately, we didn't start to rely on this unintended behavior between\n> then and now, so we let's just remove it. It isn't harming anyone, but\n> it's confusing.\n>\n> Signed-off-by: Glen Choo \n\nAs the author of the commit whose rationale was quoted above, I am\ndelighted to provide my ACK to both commit message and diff.\n\nThanks,\nJohannes\n\n> ---\n> setup.c: don't setup in discover_git_directory()\n>\n> This is the scissors patch I sent on Victoria's series [1], but rebased\n> onto \"master\" since that series hasn't been merged yet. The merge\n> conflict resolution is to delete all of the conflicting lines:\n>\n> -\tthe_repository->repository_format_worktree_config =\n> -\t\tcandidate.worktree_config;\n> -\n>\n>\n> IOW it's the original scissors patch if queued on top of Victoria's\n> series, but it might be cleaner to invert that, i.e. if we pretended\n> that this was in \"master\" already, there wouldn't be reason to add those\n> lines to begin with.\n>\n> [1]\n> https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1526%2Fchooglen%2Fpush-nknkwmnkxolv-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1526/chooglen/push-nknkwmnkxolv-v1\n> Pull-Request: https://github.com/git/git/pull/1526\n>\n> setup.c | 5 -----\n> 1 file changed, 5 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 458582207ea..bbd95f52c0f 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1423,11 +1423,6 @@ int discover_git_directory(struct strbuf *commondir,\n> \t\treturn -1;\n> \t}\n>\n> -\t/* take ownership of candidate.partial_clone */\n> -\tthe_repository->repository_format_partial_clone =\n> -\t\tcandidate.partial_clone;\n> -\tcandidate.partial_clone = NULL;\n> -\n> \tclear_repository_format(&candidate);\n> \treturn 0;\n> }\n>\n> base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n> --\n> gitgitgadget\n>\n"},{"id":"478469","messageId":"xmqq1qibs8bl.fsf@gitster.g","threadId":"59868","inReplyTo":"9a7602ba-6903-a94a-3bb5-e51c76f08058@gmx.de","subject":"Re: [PATCH] setup.c: don't setup in discover_git_directory()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-16T16:03:42Z","receivedAt":"2023-06-16T16:03:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> As explained by 16ac8b8db6 (setup: introduce the\n>> discover_git_directory() function, 2017-03-13) and the comment on its\n>> declaration, discover_git_directory() is intended to be an entrypoint\n>> into setup.c machinery that allows the Git directory to be discovered\n>> without side effects, e.g. so that read_early_config() can read\n>> \".git/config\" before the_repository has been set up.\n>>\n>> Fortunately, we didn't start to rely on this unintended behavior between\n>> then and now, so we let's just remove it. It isn't harming anyone, but\n>> it's confusing.\n>>\n>> Signed-off-by: Glen Choo \n>\n> As the author of the commit whose rationale was quoted above, I am\n> delighted to provide my ACK to both commit message and diff.\n>\n> Thanks,\n> Johannes\n\nThanks, both, for writing and reviewing.\n\nQueued.\n\n"}]}