{"thread":{"id":"57380","subject":"[PATCH 2/2] t0012: verify that built-ins handle `-h` even without gitdir","startedAt":"2022-02-08T01:05:11Z","lastAt":"2022-02-08T11:32:40Z","messageCount":8,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"447921","messageId":"49977ff9f4e5616689eecc388e53cd6312454da7.1644277797.git.gitgitgadget@gmail.com","threadId":"57380","inReplyTo":"pull.1139.git.1644277797.gitgitgadget@gmail.com","subject":"[PATCH 2/2] t0012: verify that built-ins handle `-h` even without gitdir","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-07T23:49:57Z","receivedAt":"2022-02-08T01:05:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a class of recently introduced bugs where calling, say,\n`git fetch -h` outside a repository would not show the usage but instead\nshow an ugly `BUG` message.\n\nLet's verify that this does not regress anymore.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0012-help.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 91b68c74a15..cbd725ccac8 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -139,13 +139,18 @@ test_expect_success 'git help --config-sections-for-completion' '\n '\n \n test_expect_success 'generate builtin list' '\n+\tmkdir -p sub &&\n \tgit --list-cmds=builtins >builtins\n '\n \n while read builtin\n do\n \ttest_expect_success \"$builtin can handle -h\" '\n-\t\ttest_expect_code 129 git $builtin -h >output 2>&1 &&\n+\t\t(\n+\t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t) &&\n \t\ttest_i18ngrep usage output\n \t'\n done \n\nWhen we taught these commands about the sparse index, we did not account\nfor the fact that the `cmd_*()` functions _can_ be called without a\ngitdir, namely when `-h` is passed to show the usage.\n\nA plausible approach to address this is to move the\n`prepare_repo_settings()` calls right after the `parse_options()` calls:\nThe latter will never return when it handles `-h`, and therefore it is\nsafe to assume that we have a `gitdir` at that point, as long as the\nbuilt-in is marked with the `RUN_SETUP` flag.\n\nHowever, it is unfortunately not that simple. In `cmd_pack_objects()`,\nfor example, the repo settings need to be fully populated so that the\ncommand-line options `--sparse`/`--no-sparse` can override them, not the\nother way round.\n\nTherefore, we choose to imitate the strategy taken in `cmd_diff()`,\nwhere we simply do not bother to prepare and initialize the repo\nsettings unless we have a `gitdir`.\n\nThis fixes https://github.com/git-for-windows/git/issues/3688\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 7 ++++---\n builtin/fetch.c | 7 +++++--\n builtin/pack-objects.c | 8 +++++---\n builtin/pull.c | 6 ++++--\n 4 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex cc804ba8e1e..1c13d7fedb3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1602,9 +1602,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \topts->show_progress = -1;\n \n \tgit_config(git_checkout_config, opts);\n-\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \topts->track = BRANCH_TRACK_UNSPECIFIED;\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e9..e0a05de10ee 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2014,11 +2014,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tgit_config(git_fetch_config, NULL);\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\n \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t ? &submodule_fetch_jobs_config : NULL;\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ba2006f2212..87cb7b45c37 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3976,9 +3976,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tread_replace_refs = 0;\n \n \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n-\tprepare_repo_settings(the_repository);\n-\tif (sparse < 0)\n-\t\tsparse = the_repository->settings.pack_use_sparse;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tif (sparse < 0)\n+\t\t\tsparse = the_repository->settings.pack_use_sparse;\n+\t}\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_config(git_pack_config, NULL);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 100cbf9fb85..d15007d93f3 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -994,8 +994,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tset_reflog_message(argc, argv);\n \n \tgit_config(git_pull_config, NULL);\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n-- \ngitgitgadget\n\n"},{"id":"447946","messageId":"xmqqsfstett1.fsf@gitster.g","threadId":"57380","inReplyTo":"25d1a2963f2934753de4fb6899429301637377c9.1644277797.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] checkout/fetch/pull/pack-objects: allow `-h` outside a repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-08T06:50:02Z","receivedAt":"2022-02-08T06:50:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When we taught these commands about the sparse index, we did not account\n> for the fact that the `cmd_*()` functions _can_ be called without a\n> gitdir, namely when `-h` is passed to show the usage.\n>\n> A plausible approach to address this is to move the\n> `prepare_repo_settings()` calls right after the `parse_options()` calls:\n> The latter will never return when it handles `-h`, and therefore it is\n> safe to assume that we have a `gitdir` at that point, as long as the\n> built-in is marked with the `RUN_SETUP` flag.\n>\n> However, it is unfortunately not that simple. In `cmd_pack_objects()`,\n> for example, the repo settings need to be fully populated so that the\n> command-line options `--sparse`/`--no-sparse` can override them, not the\n> other way round.\n>\n> Therefore, we choose to imitate the strategy taken in `cmd_diff()`,\n> where we simply do not bother to prepare and initialize the repo\n> settings unless we have a `gitdir`.\n>\n> This fixes https://github.com/git-for-windows/git/issues/3688\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nThese \"prepare\" calls are always tricky. This somehow reminds me of\n44c7e62e (repo-settings: prepare_repo_settings only in git repos,\n2021-12-06) and e5b17bda (git: ensure correct git directory setup\nwith -h, 2021-12-06), which were part of the ld/sparse-diff-blame\ntopic that was merged at 8d2c3732 during the last cycle.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index cc804ba8e1e..1c13d7fedb3 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1602,9 +1602,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \topts->show_progress = -1;\n> \n> \tgit_config(git_checkout_config, opts);\n> -\n> -\tprepare_repo_settings(the_repository);\n> -\tthe_repository->settings.command_requires_full_index = 0;\n> +\tif (the_repository->gitdir) {\n> +\t\tprepare_repo_settings(the_repository);\n> +\t\tthe_repository->settings.command_requires_full_index = 0;\n> +\t}\n\nLooks quite straight-forward and sensible. Of course, the checkout\ncommand will go berserk and do nonsense if it is allowed to proceed\nmuch further from here without .gitdir, but we know that \n\n (1) the only case where .gitdir is NULL at this point is when\n running \"-h\" from outside a repository (we would have done a\n full RUN_SETUP without demoting it to RUN_SETUP_GENTLY if we\n weren't doing \"-h\"), and that\n\n (2) this is soon followed by parse_options() where it notices \"-h\"\n and stops.\n\nso skipping \"prepare\" outside a repository is totally safe.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 5f06b21f8e9..e0a05de10ee 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -2014,11 +2014,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t}\n> \n> \tgit_config(git_fetch_config, NULL);\n> -\tprepare_repo_settings(the_repository);\n> -\tthe_repository->settings.command_requires_full_index = 0;\n> +\tif (the_repository->gitdir) {\n> +\t\tprepare_repo_settings(the_repository);\n> +\t\tthe_repository->settings.command_requires_full_index = 0;\n> +\t}\n\nDitto.\n\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> +\n> \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n\nUnrelated blank line?\n\n> \t\tint *sfjc = submodule_fetch_jobs_config == -1\n> \t\t\t ? &submodule_fetch_jobs_config : NULL;\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index ba2006f2212..87cb7b45c37 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3976,9 +3976,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \tread_replace_refs = 0;\n> \n> \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n> -\tprepare_repo_settings(the_repository);\n> -\tif (sparse < 0)\n> -\t\tsparse = the_repository->settings.pack_use_sparse;\n> +\tif (the_repository->gitdir) {\n> +\t\tprepare_repo_settings(the_repository);\n> +\t\tif (sparse < 0)\n> +\t\t\tsparse = the_repository->settings.pack_use_sparse;\n> +\t}\n> \n> \treset_pack_idx_option(&pack_idx_opts);\n> \tgit_config(git_pack_config, NULL);\n\nThis has a bit more sequence of calls until it reaches parse_options(),\nbut their side effect, when run outside a repository, are all benign\nin-core effects, so this is safe, too.\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 100cbf9fb85..d15007d93f3 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -994,8 +994,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \t\tset_reflog_message(argc, argv);\n> \n> \tgit_config(git_pull_config, NULL);\n> -\tprepare_repo_settings(the_repository);\n> -\tthe_repository->settings.command_requires_full_index = 0;\n> +\tif (the_repository->gitdir) {\n> +\t\tprepare_repo_settings(the_repository);\n> +\t\tthe_repository->settings.command_requires_full_index = 0;\n> +\t}\n> \n> \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n"},{"id":"447966","messageId":"pull.1139.v2.git.1644319314.gitgitgadget@gmail.com","threadId":"57380","inReplyTo":"pull.1139.git.1644277797.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] checkout/fetch/pull/pack-objects: allow -h outside a repository again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-08T11:21:52Z","receivedAt":"2022-02-08T11:32:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As reported in https://github.com/git-for-windows/git/issues/3688, calling\ngit fetch -h outside a repository now results in a very ugly\n\n BUG: repo-settings.c:23: Cannot add settings for uninitialized repository\n\n\nThe reason is that the prepare_repo_settings() calls (that we introduced to\nsupport sparse index) assume that there is a gitdir, but the hack to allow\nparse_options() to handle -h even outside a repository invalidates that\nassumption.\n\nOne strategy I considered was to move the prepare_repo_settings() calls\nafter parse_options(). This would work because when parse_options() handles\n-h, it exits without returning.\n\nHowever, this strategy failed in my tests because e.g. cmd_unpack_objects()\ndoes need the pack_use_sparse to be populated correctly before even parsing\nthe options so that it can be overridden via --sparse/--no-sparse.\n\nHence the current strategy where the code that prepares the repo settings\nand then accesses them is guarded behind the condition that we must have a\ngitdir to do so.\n\nNote: There are other instances where prepare_repo_settings() is called\nbefore parse_options(), e.g. in cmd_status(), in seen there are even more\ninstances (e.g. cmd_checkout_index()). All of those instances that are not\ntouched by this here patch do have special code to handle -h early, though,\nbefore calling prepare_repo_settings() let alone parse_options().\n\nJohannes Schindelin (2):\n checkout/fetch/pull/pack-objects: allow `-h` outside a repository\n t0012: verify that built-ins handle `-h` even without gitdir\n\n builtin/checkout.c | 7 ++++---\n builtin/fetch.c | 6 ++++--\n builtin/pack-objects.c | 8 +++++---\n builtin/pull.c | 6 ++++--\n t/t0012-help.sh | 7 ++++++-\n 5 files changed, 23 insertions(+), 11 deletions(-)\n\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1139%2Fdscho%2Fprepare_repo_settings-after-parse_options-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1139/dscho/prepare_repo_settings-after-parse_options-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1139\n\nRange-diff vs v1:\n\n 1: 25d1a2963f2 ! 1: 6e9cdd10a70 checkout/fetch/pull/pack-objects: allow `-h` outside a repository\n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n -+\n - \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n - \t\tint *sfjc = submodule_fetch_jobs_config == -1\n - \t\t\t ? &submodule_fetch_jobs_config : NULL;\n \n ## builtin/pack-objects.c ##\n @@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n 2: 49977ff9f4e = 2: 93df4a73dab t0012: verify that built-ins handle `-h` even without gitdir\n\n-- \ngitgitgadget\n"},{"id":"447968","messageId":"93df4a73dab44502eddfffd363622bf7016aaa4d.1644319314.git.gitgitgadget@gmail.com","threadId":"57380","inReplyTo":"pull.1139.v2.git.1644319314.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] t0012: verify that built-ins handle `-h` even without gitdir","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-08T11:21:54Z","receivedAt":"2022-02-08T11:32:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe just fixed a class of recently introduced bugs where calling, say,\n`git fetch -h` outside a repository would not show the usage but instead\nshow an ugly `BUG` message.\n\nLet's verify that this does not regress anymore.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0012-help.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 91b68c74a15..cbd725ccac8 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -139,13 +139,18 @@ test_expect_success 'git help --config-sections-for-completion' '\n '\n \n test_expect_success 'generate builtin list' '\n+\tmkdir -p sub &&\n \tgit --list-cmds=builtins >builtins\n '\n \n while read builtin\n do\n \ttest_expect_success \"$builtin can handle -h\" '\n-\t\ttest_expect_code 129 git $builtin -h >output 2>&1 &&\n+\t\t(\n+\t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t) &&\n \t\ttest_i18ngrep usage output\n \t'\n done \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > When we taught these commands about the sparse index, we did not account\n> > for the fact that the `cmd_*()` functions _can_ be called without a\n> > gitdir, namely when `-h` is passed to show the usage.\n> >\n> > A plausible approach to address this is to move the\n> > `prepare_repo_settings()` calls right after the `parse_options()` calls:\n> > The latter will never return when it handles `-h`, and therefore it is\n> > safe to assume that we have a `gitdir` at that point, as long as the\n> > built-in is marked with the `RUN_SETUP` flag.\n> >\n> > However, it is unfortunately not that simple. In `cmd_pack_objects()`,\n> > for example, the repo settings need to be fully populated so that the\n> > command-line options `--sparse`/`--no-sparse` can override them, not the\n> > other way round.\n> >\n> > Therefore, we choose to imitate the strategy taken in `cmd_diff()`,\n> > where we simply do not bother to prepare and initialize the repo\n> > settings unless we have a `gitdir`.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/3688\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n>\n> These \"prepare\" calls are always tricky. This somehow reminds me of\n> 44c7e62e (repo-settings: prepare_repo_settings only in git repos,\n> 2021-12-06) and e5b17bda (git: ensure correct git directory setup\n> with -h, 2021-12-06), which were part of the ld/sparse-diff-blame\n> topic that was merged at 8d2c3732 during the last cycle.\n\nYes, and I did indeed imitate the way Lessley changed `cmd_diff()`. It\nturned out to be much smarter than the first approach I tried (and that I\ndescribed as a \"plausible approach\" in the commit message).\n\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index cc804ba8e1e..1c13d7fedb3 100644\n> > --- a/builtin/checkout.c\n> > +++ b/builtin/checkout.c\n> > @@ -1602,9 +1602,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> > \topts->show_progress = -1;\n> >\n> > \tgit_config(git_checkout_config, opts);\n> > -\n> > -\tprepare_repo_settings(the_repository);\n> > -\tthe_repository->settings.command_requires_full_index = 0;\n> > +\tif (the_repository->gitdir) {\n> > +\t\tprepare_repo_settings(the_repository);\n> > +\t\tthe_repository->settings.command_requires_full_index = 0;\n> > +\t}\n>\n> Looks quite straight-forward and sensible. Of course, the checkout\n> command will go berserk and do nonsense if it is allowed to proceed\n> much further from here without .gitdir, but we know that\n>\n> (1) the only case where .gitdir is NULL at this point is when\n> running \"-h\" from outside a repository (we would have done a\n> full RUN_SETUP without demoting it to RUN_SETUP_GENTLY if we\n> weren't doing \"-h\"), and that\n>\n> (2) this is soon followed by parse_options() where it notices \"-h\"\n> and stops.\n>\n> so skipping \"prepare\" outside a repository is totally safe.\n\nIndeed.\n\nPlus, do not forget how the second patch in this series ensures that there\nare no other calls that would get the hiccups when `gitdir` is `NULL`.\n\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 5f06b21f8e9..e0a05de10ee 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -2014,11 +2014,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> > \t}\n> >\n> > \tgit_config(git_fetch_config, NULL);\n> > -\tprepare_repo_settings(the_repository);\n> > -\tthe_repository->settings.command_requires_full_index = 0;\n> > +\tif (the_repository->gitdir) {\n> > +\t\tprepare_repo_settings(the_repository);\n> > +\t\tthe_repository->settings.command_requires_full_index = 0;\n> > +\t}\n>\n> Ditto.\n>\n> > \targc = parse_options(argc, argv, prefix,\n> > \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> > +\n> > \tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n>\n> Unrelated blank line?\n\nWhoops, yes. I did mention that I first tried to move the call, right?\nThat's a left-over from not _completely_ cleaning up after that failed\nattempt.\n\n>\n> > \t\tint *sfjc = submodule_fetch_jobs_config == -1\n> > \t\t\t ? &submodule_fetch_jobs_config : NULL;\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index ba2006f2212..87cb7b45c37 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -3976,9 +3976,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > \tread_replace_refs = 0;\n> >\n> > \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n> > -\tprepare_repo_settings(the_repository);\n> > -\tif (sparse < 0)\n> > -\t\tsparse = the_repository->settings.pack_use_sparse;\n> > +\tif (the_repository->gitdir) {\n> > +\t\tprepare_repo_settings(the_repository);\n> > +\t\tif (sparse < 0)\n> > +\t\t\tsparse = the_repository->settings.pack_use_sparse;\n> > +\t}\n> >\n> > \treset_pack_idx_option(&pack_idx_opts);\n> > \tgit_config(git_pack_config, NULL);\n>\n> This has a bit more sequence of calls until it reaches parse_options(),\n> but their side effect, when run outside a repository, are all benign\n> in-core effects, so this is safe, too.\n\nAnd once again, I would like to point out that the regression test is\ncrucial to keep this benign.\n\nI'll send out v2 shortly.\n\nCiao,\nDscho\n\n>\n> > diff --git a/builtin/pull.c b/builtin/pull.c\n> > index 100cbf9fb85..d15007d93f3 100644\n> > --- a/builtin/pull.c\n> > +++ b/builtin/pull.c\n> > @@ -994,8 +994,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> > \t\tset_reflog_message(argc, argv);\n> >\n> > \tgit_config(git_pull_config, NULL);\n> > -\tprepare_repo_settings(the_repository);\n> > -\tthe_repository->settings.command_requires_full_index = 0;\n> > +\tif (the_repository->gitdir) {\n> > +\t\tprepare_repo_settings(the_repository);\n> > +\t\tthe_repository->settings.command_requires_full_index = 0;\n> > +\t}\n> >\n> > \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n>\n"},{"id":"447970","messageId":"6e9cdd10a70801da5124db1f55770d05c002671d.1644319314.git.gitgitgadget@gmail.com","threadId":"57380","inReplyTo":"pull.1139.v2.git.1644319314.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] checkout/fetch/pull/pack-objects: allow `-h` outside a repository","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-08T11:21:53Z","receivedAt":"2022-02-08T11:32:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen we taught these commands about the sparse index, we did not account\nfor the fact that the `cmd_*()` functions _can_ be called without a\ngitdir, namely when `-h` is passed to show the usage.\n\nA plausible approach to address this is to move the\n`prepare_repo_settings()` calls right after the `parse_options()` calls:\nThe latter will never return when it handles `-h`, and therefore it is\nsafe to assume that we have a `gitdir` at that point, as long as the\nbuilt-in is marked with the `RUN_SETUP` flag.\n\nHowever, it is unfortunately not that simple. In `cmd_pack_objects()`,\nfor example, the repo settings need to be fully populated so that the\ncommand-line options `--sparse`/`--no-sparse` can override them, not the\nother way round.\n\nTherefore, we choose to imitate the strategy taken in `cmd_diff()`,\nwhere we simply do not bother to prepare and initialize the repo\nsettings unless we have a `gitdir`.\n\nThis fixes https://github.com/git-for-windows/git/issues/3688\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/checkout.c | 7 ++++---\n builtin/fetch.c | 6 ++++--\n builtin/pack-objects.c | 8 +++++---\n builtin/pull.c | 6 ++++--\n 4 files changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex cc804ba8e1e..1c13d7fedb3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1602,9 +1602,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \topts->show_progress = -1;\n \n \tgit_config(git_checkout_config, opts);\n-\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \topts->track = BRANCH_TRACK_UNSPECIFIED;\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e9..c8ca4262de6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -2014,8 +2014,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tgit_config(git_fetch_config, NULL);\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ba2006f2212..87cb7b45c37 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3976,9 +3976,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tread_replace_refs = 0;\n \n \tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", -1);\n-\tprepare_repo_settings(the_repository);\n-\tif (sparse < 0)\n-\t\tsparse = the_repository->settings.pack_use_sparse;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tif (sparse < 0)\n+\t\t\tsparse = the_repository->settings.pack_use_sparse;\n+\t}\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_config(git_pack_config, NULL);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 100cbf9fb85..d15007d93f3 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -994,8 +994,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tset_reflog_message(argc, argv);\n \n \tgit_config(git_pull_config, NULL);\n-\tprepare_repo_settings(the_repository);\n-\tthe_repository->settings.command_requires_full_index = 0;\n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n \n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n-- \ngitgitgadget\n\n"}]}