{"thread":{"id":"60587","subject":"[PATCH 0/7] clone: fix init of refdb with wrong object format","startedAt":"2023-12-06T12:39:52Z","lastAt":"2023-12-12T19:20:51Z","messageCount":33,"participants":["Patrick Steinhardt","Karthik Nayak","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"485370","messageId":"cover.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":null,"subject":"[PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:39:44Z","receivedAt":"2023-12-06T12:39:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwhen using git-clone(1), we initialize the complete repository before we\nknow about the object format used by the remote repository. This means\nthat we'll potentially create the refdb with the wrong object format in\ncase the local default object format and remote object format are not\nthe same.\n\nThis isn't much of a problem in the context of the files backend, which\nnever records the object format anyway. But it is a problem for the\nreftable backend, which indeed records the object format in the on-disk\ndata structures. The result is thus a reftable with wrong object format.\n\nThis patch series aims to solve this issue by initializing the refdb at\na later point after we have learned about the remote object format. This\nrequires some careful reordering of code. Unfortunately, the end result\nis not easily verifiable and thus I didn't add tests here. But it does\nfix cloning of SHA256 repositories with the in-progress state of the\nreftable backend.\n\nWhile at it I noticed that this actually fixes a bug with bundle URIs\nwhen the object formats diverge in this way.\n\nThe series is layed out as follows:\n\n - Patch 1 + 2: split out a function to create the refdb and make it\n possible to skip its initialization in `init_db()`.\n\n - Patch 3: allows git-remote-curl(1) to work with repos that get\n initialized during its lifetime.\n\n - Patch 4 - 6: address various corner cases where we access the refdb\n before we learned about the object format.\n\n - Patch 7: move initialization of the refdb to happen after we have\n learned about the object format.\n\nThis patch series is actually the last incompatibility for the reftable\nbackend that I have found. All tests except for the files-backend\nspecific ones pass now with the current state I have at [1], which is\ncurrently at e6f2f592b7 (t: skip tests which are incompatible with\nreftable, 2023-11-24)\n\nThanks in advance for your reviews!\n\nPatrick\n\nPatrick Steinhardt (7):\n setup: extract function to create the refdb\n setup: allow skipping creation of the refdb\n remote-curl: rediscover repository when fetching refs\n builtin/clone: fix bundle URIs with mismatching object formats\n builtin/clone: set up sparse checkout later\n builtin/clone: skip reading HEAD when retrieving remote\n builtin/clone: create the refdb with the correct object format\n\n builtin/clone.c | 65 ++++++++++++----------\n remote-curl.c | 7 ++-\n remote.c | 26 +++++----\n remote.h | 1 +\n setup.c | 106 +++++++++++++++++++++---------------\n setup.h | 6 +-\n t/t5558-clone-bundle-uri.sh | 18 ++++++\n 7 files changed, 140 insertions(+), 89 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"485371","messageId":"b69c57d27269c9b40c9e4394861dffd8a8b9860c.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 1/7] setup: extract function to create the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:39:48Z","receivedAt":"2023-12-06T12:39:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to let callers skip creation of the reference database when\ncalling `init_db()`. Extract the logic into a standalone function so\nthat it becomes easier to do this refactoring.\n\nSigned-off-by: Patrick Steinhardt \n---\n setup.c | 95 ++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 57 insertions(+), 38 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..9fcb64159f 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1885,6 +1885,60 @@ void initialize_repository_version(int hash_algo, int reinit)\n \t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n }\n \n+static int is_reinit(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar junk[2];\n+\tint ret;\n+\n+\tgit_path_buf(&buf, \"HEAD\");\n+\tret = !access(buf.buf, R_OK) || readlink(buf.buf, junk, sizeof(junk) - 1) != -1;\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static void create_reference_database(const char *initial_branch, int quiet)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint reinit = is_reinit();\n+\n+\t/*\n+\t * We need to create a \"refs\" dir in any case so that older\n+\t * versions of git can tell that this is a repository.\n+\t */\n+\tsafe_create_dir(git_path(\"refs\"), 1);\n+\tadjust_shared_perm(git_path(\"refs\"));\n+\n+\tif (refs_init_db(&err))\n+\t\tdie(\"failed to set up refs db: %s\", err.buf);\n+\n+\t/*\n+\t * Point the HEAD symref to the initial branch with if HEAD does\n+\t * not yet exist.\n+\t */\n+\tif (!reinit) {\n+\t\tchar *ref;\n+\n+\t\tif (!initial_branch)\n+\t\t\tinitial_branch = git_default_branch_name(quiet);\n+\n+\t\tref = xstrfmt(\"refs/heads/%s\", initial_branch);\n+\t\tif (check_refname_format(ref, 0) < 0)\n+\t\t\tdie(_(\"invalid initial branch name: '%s'\"),\n+\t\t\t initial_branch);\n+\n+\t\tif (create_symref(\"HEAD\", ref, NULL) < 0)\n+\t\t\texit(1);\n+\t\tfree(ref);\n+\t}\n+\n+\tif (reinit && initial_branch)\n+\t\twarning(_(\"re-init: ignored --initial-branch=%s\"),\n+\t\t\tinitial_branch);\n+\n+\tstrbuf_release(&err);\n+}\n+\n static int create_default_files(const char *template_path,\n \t\t\t\tconst char *original_git_dir,\n \t\t\t\tconst char *initial_branch,\n@@ -1896,10 +1950,8 @@ static int create_default_files(const char *template_path,\n \tstruct stat st1;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *path;\n-\tchar junk[2];\n \tint reinit;\n \tint filemode;\n-\tstruct strbuf err = STRBUF_INIT;\n \tconst char *init_template_dir = NULL;\n \tconst char *work_tree = get_git_work_tree();\n \n@@ -1919,6 +1971,8 @@ static int create_default_files(const char *template_path,\n \treset_shared_repository();\n \tgit_config(git_default_config, NULL);\n \n+\treinit = is_reinit();\n+\n \t/*\n \t * We must make sure command-line options continue to override any\n \t * values we might have just re-read from the config.\n@@ -1962,39 +2016,7 @@ static int create_default_files(const char *template_path,\n \t\tadjust_shared_perm(get_git_dir());\n \t}\n \n-\t/*\n-\t * We need to create a \"refs\" dir in any case so that older\n-\t * versions of git can tell that this is a repository.\n-\t */\n-\tsafe_create_dir(git_path(\"refs\"), 1);\n-\tadjust_shared_perm(git_path(\"refs\"));\n-\n-\tif (refs_init_db(&err))\n-\t\tdie(\"failed to set up refs db: %s\", err.buf);\n-\n-\t/*\n-\t * Point the HEAD symref to the initial branch with if HEAD does\n-\t * not yet exist.\n-\t */\n-\tpath = git_path_buf(&buf, \"HEAD\");\n-\treinit = (!access(path, R_OK)\n-\t\t || readlink(path, junk, sizeof(junk)-1) != -1);\n-\tif (!reinit) {\n-\t\tchar *ref;\n-\n-\t\tif (!initial_branch)\n-\t\t\tinitial_branch = git_default_branch_name(quiet);\n-\n-\t\tref = xstrfmt(\"refs/heads/%s\", initial_branch);\n-\t\tif (check_refname_format(ref, 0) < 0)\n-\t\t\tdie(_(\"invalid initial branch name: '%s'\"),\n-\t\t\t initial_branch);\n-\n-\t\tif (create_symref(\"HEAD\", ref, NULL) < 0)\n-\t\t\texit(1);\n-\t\tfree(ref);\n-\t}\n-\n+\tcreate_reference_database(initial_branch, quiet);\n \tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n@@ -2158,9 +2180,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t\t\t prev_bare_repository,\n \t\t\t\t init_shared_repository,\n \t\t\t\t flags & INIT_DB_QUIET);\n-\tif (reinit && initial_branch)\n-\t\twarning(_(\"re-init: ignored --initial-branch=%s\"),\n-\t\t\tinitial_branch);\n \n \tcreate_object_directory();\n \n-- \n2.43.0\n\n"},{"id":"485372","messageId":"090c52423e3de9c4e94c6694bb18fb8510a3ad08.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 2/7] setup: allow skipping creation of the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:39:53Z","receivedAt":"2023-12-06T12:39:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Allow callers to skip creation of the reference database via a new flag\n`INIT_DB_SKIP_REFDB`, which is required for git-clone(1) so that we can\ncreate it at a later point once the object format has been discovered\nfrom the remote repository.\n\nNote that we also uplift the call to `create_reference_database()` into\n`init_db()`, which makes it easier to handle the new flag for us. This\nchanges the order in which we do initialization so that we now set up\nthe Git configuration before we create the reference database. In\npractice this move should not result in any change in behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n setup.c | 13 +++++--------\n setup.h | 5 +++--\n 2 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 9fcb64159f..a80fc09b9c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1941,11 +1941,9 @@ static void create_reference_database(const char *initial_branch, int quiet)\n \n static int create_default_files(const char *template_path,\n \t\t\t\tconst char *original_git_dir,\n-\t\t\t\tconst char *initial_branch,\n \t\t\t\tconst struct repository_format *fmt,\n \t\t\t\tint prev_bare_repository,\n-\t\t\t\tint init_shared_repository,\n-\t\t\t\tint quiet)\n+\t\t\t\tint init_shared_repository)\n {\n \tstruct stat st1;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -2016,7 +2014,6 @@ static int create_default_files(const char *template_path,\n \t\tadjust_shared_perm(get_git_dir());\n \t}\n \n-\tcreate_reference_database(initial_branch, quiet);\n \tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n@@ -2176,11 +2173,11 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \n \treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t initial_branch, &repo_fmt,\n-\t\t\t\t prev_bare_repository,\n-\t\t\t\t init_shared_repository,\n-\t\t\t\t flags & INIT_DB_QUIET);\n+\t\t\t\t &repo_fmt, prev_bare_repository,\n+\t\t\t\t init_shared_repository);\n \n+\tif (!(flags & INIT_DB_SKIP_REFDB))\n+\t\tcreate_reference_database(initial_branch, flags & INIT_DB_QUIET);\n \tcreate_object_directory();\n \n \tif (get_shared_repository()) {\ndiff --git a/setup.h b/setup.h\nindex b48cf1c43b..cbf538286b 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -169,8 +169,9 @@ int verify_repository_format(const struct repository_format *format,\n */\n void check_repository_format(struct repository_format *fmt);\n \n-#define INIT_DB_QUIET 0x0001\n-#define INIT_DB_EXIST_OK 0x0002\n+#define INIT_DB_QUIET (1 << 0)\n+#define INIT_DB_EXIST_OK (1 << 1)\n+#define INIT_DB_SKIP_REFDB (1 << 2)\n \n int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *template_dir, int hash_algo,\n-- \n2.43.0\n\n"},{"id":"485373","messageId":"a1b86a0cbbedcc6610b2c563e9e38d439338869d.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 3/7] remote-curl: rediscover repository when fetching refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:39:57Z","receivedAt":"2023-12-06T12:40:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to change git-clone(1) so that we set up the reference\ndatabase at a later point. This change will cause git-remote-curl(1) to\nnot detect the repository anymore due to \"HEAD\" not having been created\nyet at the time it spawns, and thus causes it to error out once it is\nasked to fetch the references.\n\nWe can address this issue by trying to re-discover the Git repository in\ncase none was detected at startup time. With this change, the clone will\nlook as following:\n\n 1. git-clone(1) sets up the initial repository, excluding the\n reference database.\n\n 2. git-clone(1) spawns git-remote-curl(1), which will be unable to\n detect the repository due to a missing \"HEAD\".\n\n 3. git-clone(1) asks git-remote-curl(1) to list remote references.\n This works just fine as this step does not require a local\n repository\n\n 4. git-clone(1) creates the reference database as it has now learned\n about the object format.\n\n 5. git-clone(1) asks git-remote-curl(1) to fetch the remote packfile.\n The latter notices that it doesn't have a repository available, but\n it now knows to try and re-discover it.\n\nIf the re-discovery succeeds in the last step we can continue with the\nclone.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote-curl.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..fc29757b65 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1564,8 +1564,11 @@ int cmd_main(int argc, const char **argv)\n \t\tif (buf.len == 0)\n \t\t\tbreak;\n \t\tif (starts_with(buf.buf, \"fetch \")) {\n-\t\t\tif (nongit)\n-\t\t\t\tdie(_(\"remote-curl: fetch attempted without a local repo\"));\n+\t\t\tif (nongit) {\n+\t\t\t\tsetup_git_directory_gently(&nongit);\n+\t\t\t\tif (nongit)\n+\t\t\t\t\tdie(_(\"remote-curl: fetch attempted without a local repo\"));\n+\t\t\t}\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n-- \n2.43.0\n\n"},{"id":"485374","messageId":"c7a9d6ef74ff39e660f80e2e104a96b7c875845d.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:40:01Z","receivedAt":"2023-12-06T12:40:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We create the reference database in git-clone(1) quite early before\nconnecting to the remote repository. Given that we do not yet know about\nthe object format that the remote repository uses at that point in time\nthe consequence is that the refdb may be initialized with the wrong\nobject format.\n\nThis is not a problem in the context of the files backend as we do not\nencode the object format anywhere, and furthermore the only reference\nthat we write between initializing the refdb and learning about the\nobject format is the \"HEAD\" symref. It will become a problem though once\nwe land the reftable backend, which indeed does require to know about\nthe proper object format at the time of creation. We thus need to\nrearrange the logic in git-clone(1) so that we only initialize the refdb\nonce we have learned about the actual object format.\n\nAs a first step, move listing of remote references to happen earlier,\nwhich also allow us to set up the hash algorithm of the repository\nearlier now. While we aim to execute this logic as late as possible\nuntil after most of the setup has happened already, detection of the\nobject format and thus later the setup of the reference database must\nhappen before any other logic that may spawn Git commands or otherwise\nthese Git commands may not recognize the repository as such.\n\nThe first Git step where we expect the repository to be fully initalized\nis when we fetch bundles via bundle URIs. Funny enough, the comments\nthere also state that \"the_repository must match the cloned repo\", which\nis indeed not necessarily the case for the hash algorithm right now. So\nin practice it is the right thing to detect the remote's object format\nbefore downloading bundle URIs anyway, and not doing so causes clones\nwith bundle URIS to fail when the local default object format does not\nmatch the remote repository's format.\n\nUnfortunately though, this creates a new issue: downloading bundles may\ntake a long time, so if we list refs beforehand they might've grown\nstale meanwhile. It is not clear how to solve this issue except for a\nsecond reference listing though after we have downloaded the bundles,\nwhich may be an expensive thing to do.\n\nArguably though, it's preferable to have a staleness issue compared to\nbeing unable to clone a repository altogether.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 48 ++++++++++++++++++-------------------\n t/t5558-clone-bundle-uri.sh | 18 ++++++++++++++\n 2 files changed, 41 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af949..d188650881 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1266,6 +1266,26 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (transport->smart_options && !deepen && !filter_options.choice)\n \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \n+\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n+\trefspec_ref_prefixes(&remote->fetch,\n+\t\t\t &transport_ls_refs_options.ref_prefixes);\n+\tif (option_branch)\n+\t\texpand_ref_prefix(&transport_ls_refs_options.ref_prefixes,\n+\t\t\t\t option_branch);\n+\tif (!option_no_tags)\n+\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n+\t\t\t \"refs/tags/\");\n+\n+\trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n+\n+\t/*\n+\t * Now that we know what algorithm the remote side is using, let's set\n+\t * ours to the same thing.\n+\t */\n+\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n+\tinitialize_repository_version(hash_algo, 1);\n+\trepo_set_hash_algo(the_repository, hash_algo);\n+\n \t/*\n \t * Before fetching from the remote, download and install bundle\n \t * data from the --bundle-uri option.\n@@ -1281,24 +1301,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\tbundle_uri);\n \t\telse if (has_heuristic)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n-\t}\n-\n-\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n-\trefspec_ref_prefixes(&remote->fetch,\n-\t\t\t &transport_ls_refs_options.ref_prefixes);\n-\tif (option_branch)\n-\t\texpand_ref_prefix(&transport_ls_refs_options.ref_prefixes,\n-\t\t\t\t option_branch);\n-\tif (!option_no_tags)\n-\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n-\t\t\t \"refs/tags/\");\n-\n-\trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n-\n-\tif (refs)\n-\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n-\n-\tif (!bundle_uri) {\n+\t} else {\n \t\t/*\n \t\t* Populate transport->got_remote_bundle_uri and\n \t\t* transport->bundle_uri. We might get nothing.\n@@ -1319,13 +1322,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\t\t/*\n-\t\t * Now that we know what algorithm the remote side is using,\n-\t\t * let's set ours to the same thing.\n-\t\t */\n-\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n-\tinitialize_repository_version(hash_algo, 1);\n-\trepo_set_hash_algo(the_repository, hash_algo);\n+\tif (refs)\n+\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n \n \tif (mapped_refs) {\n \t\t/*\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex 996a08e90c..1ca5f745e7 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -33,6 +33,15 @@ test_expect_success 'clone with path bundle' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone with path bundle and non-default hash' '\n+\ttest_when_finished \"rm -rf clone-path-non-default-hash\" &&\n+\tGIT_DEFAULT_HASH=sha256 git clone --bundle-uri=\"clone-from/B.bundle\" \\\n+\t\tclone-from clone-path-non-default-hash &&\n+\tgit -C clone-path-non-default-hash rev-parse refs/bundles/topic >actual &&\n+\tgit -C clone-from rev-parse topic >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone with file:// bundle' '\n \tgit clone --bundle-uri=\"file://$(pwd)/clone-from/B.bundle\" \\\n \t\tclone-from clone-file &&\n@@ -284,6 +293,15 @@ test_expect_success 'clone HTTP bundle' '\n \ttest_config -C clone-http log.excludedecoration refs/bundle/\n '\n \n+test_expect_success 'clone HTTP bundle with non-default hash' '\n+\ttest_when_finished \"rm -rf clone-http-non-default-hash\" &&\n+\tGIT_DEFAULT_HASH=sha256 git clone --bundle-uri=\"$HTTPD_URL/B.bundle\" \\\n+\t\t\"$HTTPD_URL/smart/fetch.git\" clone-http-non-default-hash &&\n+\tgit -C clone-http-non-default-hash rev-parse refs/bundles/topic >actual &&\n+\tgit -C clone-from rev-parse topic >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone bundle list (HTTP, no heuristic)' '\n \ttest_when_finished rm -f trace*.txt &&\n \n-- \n2.43.0\n\n"},{"id":"485375","messageId":"703ff77eea6daed0d44908e6a8a20aaa64af99d6.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 5/7] builtin/clone: set up sparse checkout later","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:40:06Z","receivedAt":"2023-12-06T12:40:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When asked to do a sparse checkout, then git-clone(1) will spawn\n`git sparse-checkout set` to set up the configuration accordingly. This\nrequires a proper Git repository or otherwise the command will fail. But\nas we are about to move creation of the reference database to a later\npoint, this prerequisite will not hold anymore.\n\nMove the logic to a later point in time where we know to have created\nthe reference database already.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex d188650881..9c60923f31 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1185,9 +1185,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n-\t\treturn 1;\n-\n \tremote = remote_get(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n@@ -1426,6 +1423,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdissociate_from_references();\n \t}\n \n+\tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n+\t\treturn 1;\n+\n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout(submodule_progress, filter_submodules);\n \n-- \n2.43.0\n\n"},{"id":"485376","messageId":"6c919fb19c4e0a723c23b407717fda7649bb360f.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 6/7] builtin/clone: skip reading HEAD when retrieving remote","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:40:10Z","receivedAt":"2023-12-06T12:40:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"After we have set up the remote configuration in git-clone(1) we'll call\n`remote_get()` to read the remote from the on-disk configuration. But\nnext to reading the on-disk configuration, `remote_get()` will also\ncause us to try and read the repository's HEAD reference so that we can\nfigure out the current branch. Besides being pointless in git-clone(1)\nbecause we're operating in an empty repository anyway, this will also\nbreak once we move creation of the reference database to a later point\nin time.\n\nRefactor the code to introduce a new `remote_get_early()` function that\nwill skip reading the HEAD reference to address this issue.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 2 +-\n remote.c | 26 ++++++++++++++++----------\n remote.h | 1 +\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9c60923f31..06966c5d4c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1185,7 +1185,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\tremote = remote_get(remote_name);\n+\tremote = remote_get_early(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n \t\t\tbranch_top.buf);\ndiff --git a/remote.c b/remote.c\nindex abb24822be..051d0a64a0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -509,7 +509,7 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t}\n }\n \n-static void read_config(struct repository *repo)\n+static void read_config(struct repository *repo, int early)\n {\n \tint flag;\n \n@@ -518,7 +518,7 @@ static void read_config(struct repository *repo)\n \trepo->remote_state->initialized = 1;\n \n \trepo->remote_state->current_branch = NULL;\n-\tif (startup_info->have_repository) {\n+\tif (startup_info->have_repository && !early) {\n \t\tconst char *head_ref = refs_resolve_ref_unsafe(\n \t\t\tget_main_ref_store(repo), \"HEAD\", 0, NULL, &flag);\n \t\tif (head_ref && (flag & REF_ISSYMREF) &&\n@@ -561,7 +561,7 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,\n \n const char *remote_for_branch(struct branch *branch, int *explicit)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \treturn remotes_remote_for_branch(the_repository->remote_state, branch,\n@@ -587,7 +587,7 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,\n \n const char *pushremote_for_branch(struct branch *branch, int *explicit)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \treturn remotes_pushremote_for_branch(the_repository->remote_state,\n@@ -599,7 +599,7 @@ static struct remote *remotes_remote_get(struct remote_state *remote_state,\n \n const char *remote_ref_for_branch(struct branch *branch, int for_push)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \tif (branch) {\n@@ -709,7 +709,13 @@ remotes_remote_get(struct remote_state *remote_state, const char *name)\n \n struct remote *remote_get(const char *name)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n+\treturn remotes_remote_get(the_repository->remote_state, name);\n+}\n+\n+struct remote *remote_get_early(const char *name)\n+{\n+\tread_config(the_repository, 1);\n \treturn remotes_remote_get(the_repository->remote_state, name);\n }\n \n@@ -722,7 +728,7 @@ remotes_pushremote_get(struct remote_state *remote_state, const char *name)\n \n struct remote *pushremote_get(const char *name)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \treturn remotes_pushremote_get(the_repository->remote_state, name);\n }\n \n@@ -738,7 +744,7 @@ int remote_is_configured(struct remote *remote, int in_repo)\n int for_each_remote(each_remote_fn fn, void *priv)\n {\n \tint i, result = 0;\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tfor (i = 0; i < the_repository->remote_state->remotes_nr && !result;\n \t i++) {\n \t\tstruct remote *remote =\n@@ -1831,7 +1837,7 @@ struct branch *branch_get(const char *name)\n {\n \tstruct branch *ret;\n \n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tif (!name || !*name || !strcmp(name, \"HEAD\"))\n \t\tret = the_repository->remote_state->current_branch;\n \telse\n@@ -1973,7 +1979,7 @@ static const char *branch_get_push_1(struct remote_state *remote_state,\n \n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \tif (!branch)\ndiff --git a/remote.h b/remote.h\nindex cdc8b1db42..79353ba226 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -118,6 +118,7 @@ struct remote {\n * and configuration.\n */\n struct remote *remote_get(const char *name);\n+struct remote *remote_get_early(const char *name);\n \n struct remote *pushremote_get(const char *name);\n int remote_is_configured(struct remote *remote, int in_repo);\n-- \n2.43.0\n\n"},{"id":"485377","messageId":"eb5530e6a8760c5023340766c0103f1a7a2ebf3b.1701863960.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH 7/7] builtin/clone: create the refdb with the correct object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T12:40:15Z","receivedAt":"2023-12-06T12:40:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're currently creating the reference database with a potentially\nincorrect object format when the remote repository's object format is\ndifferent from the local default object format. This works just fine for\nnow because the files backend never records the object format anywhere.\nBut this logic will fail with any new reference backend that encodes\nthis information in some form either on-disk or in-memory.\n\nThe preceding commits have reshuffled code in git-clone(1) so that there\nis no code path that will access the reference database before we have\ndetected the remote's object format. With these refactorings we can now\ndefer initialization of the reference database until after we have\nlearned the remote's object format and thus initialize it with the\ncorrect format from the get-go.\n\nThese refactorings are required to make git-clone(1) work with the\nupcoming reftable backend when cloning repositories with the SHA256\nobject format.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 9 ++++++++-\n setup.c | 2 +-\n setup.h | 1 +\n 3 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 06966c5d4c..fd052b8b54 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1097,8 +1097,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\t/*\n+\t * Initialize the repository, but skip initializing the reference\n+\t * database. We do not yet know about the object format of the\n+\t * repository, and reference backends may persist that information into\n+\t * their on-disk data structures.\n+\t */\n \tinit_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,\n-\t\tdo_not_override_repo_unix_permissions, INIT_DB_QUIET);\n+\t\tdo_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);\n \n \tif (real_git_dir) {\n \t\tfree((char *)git_dir);\n@@ -1282,6 +1288,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n \tinitialize_repository_version(hash_algo, 1);\n \trepo_set_hash_algo(the_repository, hash_algo);\n+\tcreate_reference_database(NULL, 1);\n \n \t/*\n \t * Before fetching from the remote, download and install bundle\ndiff --git a/setup.c b/setup.c\nindex a80fc09b9c..e1d0ce29c6 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1897,7 +1897,7 @@ static int is_reinit(void)\n \treturn ret;\n }\n \n-static void create_reference_database(const char *initial_branch, int quiet)\n+void create_reference_database(const char *initial_branch, int quiet)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tint reinit = is_reinit();\ndiff --git a/setup.h b/setup.h\nindex cbf538286b..3f0f17c351 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -178,6 +178,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *initial_branch, int init_shared_repository,\n \t unsigned int flags);\n void initialize_repository_version(int hash_algo, int reinit);\n+void create_reference_database(const char *initial_branch, int quiet);\n \n /*\n * NOTE NOTE NOTE!!\n-- \n2.43.0\n\n"},{"id":"485381","messageId":"ZXByiXj5rLUHgoAU@tanuki","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-06T13:09:29Z","receivedAt":"2023-12-06T13:09:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 01:39:44PM +0100, Patrick Steinhardt wrote:\n> Hi,\n> \n> when using git-clone(1), we initialize the complete repository before we\n> know about the object format used by the remote repository. This means\n> that we'll potentially create the refdb with the wrong object format in\n> case the local default object format and remote object format are not\n> the same.\n> \n> This isn't much of a problem in the context of the files backend, which\n> never records the object format anyway. But it is a problem for the\n> reftable backend, which indeed records the object format in the on-disk\n> data structures. The result is thus a reftable with wrong object format.\n> \n> This patch series aims to solve this issue by initializing the refdb at\n> a later point after we have learned about the remote object format. This\n> requires some careful reordering of code. Unfortunately, the end result\n> is not easily verifiable and thus I didn't add tests here. But it does\n> fix cloning of SHA256 repositories with the in-progress state of the\n> reftable backend.\n> \n> While at it I noticed that this actually fixes a bug with bundle URIs\n> when the object formats diverge in this way.\n> \n> The series is layed out as follows:\n> \n> - Patch 1 + 2: split out a function to create the refdb and make it\n> possible to skip its initialization in `init_db()`.\n> \n> - Patch 3: allows git-remote-curl(1) to work with repos that get\n> initialized during its lifetime.\n> \n> - Patch 4 - 6: address various corner cases where we access the refdb\n> before we learned about the object format.\n> \n> - Patch 7: move initialization of the refdb to happen after we have\n> learned about the object format.\n> \n> This patch series is actually the last incompatibility for the reftable\n> backend that I have found. All tests except for the files-backend\n> specific ones pass now with the current state I have at [1], which is\n> currently at e6f2f592b7 (t: skip tests which are incompatible with\n> reftable, 2023-11-24)\n\nI forgot to add the link to the merge request that contains the current\nreftable backend's implementation in case anybody is interested:\nhttps://gitlab.com/gitlab-org/git/-/merge_requests/58.\n\nPatrick\n\n> Thanks in advance for your reviews!\n> \n> Patrick\n> \n> Patrick Steinhardt (7):\n> setup: extract function to create the refdb\n> setup: allow skipping creation of the refdb\n> remote-curl: rediscover repository when fetching refs\n> builtin/clone: fix bundle URIs with mismatching object formats\n> builtin/clone: set up sparse checkout later\n> builtin/clone: skip reading HEAD when retrieving remote\n> builtin/clone: create the refdb with the correct object format\n> \n> builtin/clone.c | 65 ++++++++++++----------\n> remote-curl.c | 7 ++-\n> remote.c | 26 +++++----\n> remote.h | 1 +\n> setup.c | 106 +++++++++++++++++++++---------------\n> setup.h | 6 +-\n> t/t5558-clone-bundle-uri.sh | 18 ++++++\n> 7 files changed, 140 insertions(+), 89 deletions(-)\n> \n> -- \n> 2.43.0\n> \n\n\n"},{"id":"485397","messageId":"CAOLa=ZSZztJUF9nmSzGdOW0oWBRUp2sw8QyuZO_q06cNymad3Q@mail.gmail.com","threadId":"60587","inReplyTo":"b69c57d27269c9b40c9e4394861dffd8a8b9860c.1701863960.git.ps@pks.im","subject":"Re: [PATCH 1/7] setup: extract function to create the refdb","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-12-06T21:10:37Z","receivedAt":"2023-12-06T21:11:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n> +static void create_reference_database(const char *initial_branch, int quiet)\n> +{\n> + struct strbuf err = STRBUF_INIT;\n> + int reinit = is_reinit();\n> +\n> + /*\n> + * We need to create a \"refs\" dir in any case so that older\n> + * versions of git can tell that this is a repository.\n> + */\n\nHow does this work though, even if an earlier version of git can tell\nthat this is a repository,\nit still won't be able to read the reftable backend. In that sense,\nwhat do we achieve here?\n\n> + safe_create_dir(git_path(\"refs\"), 1);\n> + adjust_shared_perm(git_path(\"refs\"));\n> +\n\nNot related to your commit per se, but we ignore the return value\nhere, shouldn't we die in this case?\n"},{"id":"485398","messageId":"CAOLa=ZQc=7Z3w9JAdzS23P=c=KSYZJR6gJSLOHdU-d92Y3kJ5A@mail.gmail.com","threadId":"60587","inReplyTo":"c7a9d6ef74ff39e660f80e2e104a96b7c875845d.1701863960.git.ps@pks.im","subject":"Re: [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-12-06T21:13:43Z","receivedAt":"2023-12-06T21:14:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n> The first Git step where we expect the repository to be fully initalized\n> is when we fetch bundles via bundle URIs. Funny enough, the comments\n> there also state that \"the_repository must match the cloned repo\", which\n> is indeed not necessarily the case for the hash algorithm right now. So\n> in practice it is the right thing to detect the remote's object format\n> before downloading bundle URIs anyway, and not doing so causes clones\n> with bundle URIS to fail when the local default object format does not\n> match the remote repository's format.\n>\n\nNit: s/URIS/URIs\n"},{"id":"485424","messageId":"ZXFyylqzNgXwc4Lz@tanuki","threadId":"60587","inReplyTo":"CAOLa=ZQc=7Z3w9JAdzS23P=c=KSYZJR6gJSLOHdU-d92Y3kJ5A@mail.gmail.com","subject":"Re: [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T07:22:50Z","receivedAt":"2023-12-07T07:22:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 10:13:43PM +0100, Karthik Nayak wrote:\n> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n> > The first Git step where we expect the repository to be fully initalized\n> > is when we fetch bundles via bundle URIs. Funny enough, the comments\n> > there also state that \"the_repository must match the cloned repo\", which\n> > is indeed not necessarily the case for the hash algorithm right now. So\n> > in practice it is the right thing to detect the remote's object format\n> > before downloading bundle URIs anyway, and not doing so causes clones\n> > with bundle URIS to fail when the local default object format does not\n> > match the remote repository's format.\n> >\n> \n> Nit: s/URIS/URIs\n\nThanks, fixed locally. Will wait with v2 though until there are more\nreview comments.\n\nPatrick\n"},{"id":"485425","messageId":"ZXFy0_T1AZLh058g@tanuki","threadId":"60587","inReplyTo":"CAOLa=ZSZztJUF9nmSzGdOW0oWBRUp2sw8QyuZO_q06cNymad3Q@mail.gmail.com","subject":"Re: [PATCH 1/7] setup: extract function to create the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T07:22:59Z","receivedAt":"2023-12-07T07:23:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:\n> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n> > +static void create_reference_database(const char *initial_branch, int quiet)\n> > +{\n> > + struct strbuf err = STRBUF_INIT;\n> > + int reinit = is_reinit();\n> > +\n> > + /*\n> > + * We need to create a \"refs\" dir in any case so that older\n> > + * versions of git can tell that this is a repository.\n> > + */\n> \n> How does this work though, even if an earlier version of git can tell\n> that this is a repository, it still won't be able to read the reftable\n> backend. In that sense, what do we achieve here?\n\nThis is a good question, and there is related ongoing discussion about\nthis topic in the thread starting at [1]. There are a few benefits to\nletting clients discover such repos even if they don't understand the\nnew reference backend format:\n\n - They know to stop walking up the parent-directory chain. Otherwise a\n client might end up detecting a Git repository in the parent dir.\n\n - The user gets a proper error message why the repository cannot be\n accessed. Instead of failing to detect the repository altogether we\n instead say that we don't understand the \"extensions.refFormat\"\n extension.\n\nMaybe there are other cases I can't think of right now.\n\n> > + safe_create_dir(git_path(\"refs\"), 1);\n> > + adjust_shared_perm(git_path(\"refs\"));\n> > +\n> \n> Not related to your commit per se, but we ignore the return value\n> here, shouldn't we die in this case?\n\nWhile the end result wouldn't be quite what the user asks for, the only\nnegative consequence is that the repository is inaccessible to others. I\nthink this failure mode is comparatively benign -- if it were the other\nway round and we'd over-share the repository it would more severe.\n\nSo while I don't think that dying makes much sense here, I could\ncertainly see us adding a warning so that the user at least knows that\nsomething went wrong. I'd rather want to keep this out of the current\npatch series, but could certainly see such a warning added in a follow\nup patch series.\n\nPatrick\n\n[1]: \n"},{"id":"485493","messageId":"xmqq1qbw1f0j.fsf@gitster.g","threadId":"60587","inReplyTo":"ZXFy0_T1AZLh058g@tanuki","subject":"Re: [PATCH 1/7] setup: extract function to create the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T22:54:52Z","receivedAt":"2023-12-08T22:54:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:\n>> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n>> > +static void create_reference_database(const char *initial_branch, int quiet)\n>> > +{\n>> > + struct strbuf err = STRBUF_INIT;\n>> > + int reinit = is_reinit();\n>> > +\n>> > + /*\n>> > + * We need to create a \"refs\" dir in any case so that older\n>> > + * versions of git can tell that this is a repository.\n>> > + */\n>> \n>> How does this work though, even if an earlier version of git can tell\n>> that this is a repository, it still won't be able to read the reftable\n>> backend. In that sense, what do we achieve here?\n>\n> This is a good question, and there is related ongoing discussion about\n> this topic in the thread starting at [1]. There are a few benefits to\n> letting clients discover such repos even if they don't understand the\n> new reference backend format:\n>\n> - They know to stop walking up the parent-directory chain. Otherwise a\n> client might end up detecting a Git repository in the parent dir.\n>\n> - The user gets a proper error message why the repository cannot be\n> accessed. Instead of failing to detect the repository altogether we\n> instead say that we don't understand the \"extensions.refFormat\"\n> extension.\n\nYup, both are very good reasons. Would it help to sneak a condensed\nversion of it in the in-code comment, perhaps?\n"},{"id":"485495","messageId":"xmqqmsukz3yr.fsf@gitster.g","threadId":"60587","inReplyTo":"a1b86a0cbbedcc6610b2c563e9e38d439338869d.1701863960.git.ps@pks.im","subject":"Re: [PATCH 3/7] remote-curl: rediscover repository when fetching refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T23:09:32Z","receivedAt":"2023-12-08T23:09:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We're about to change git-clone(1) so that we set up the reference\n> database at a later point. This change will cause git-remote-curl(1) to\n> not detect the repository anymore due to \"HEAD\" not having been created\n> yet at the time it spawns, and thus causes it to error out once it is\n> asked to fetch the references.\n>\n> We can address this issue by trying to re-discover the Git repository in\n> case none was detected at startup time. With this change, the clone will\n> look as following:\n>\n> 1. git-clone(1) sets up the initial repository, excluding the\n> reference database.\n>\n> 2. git-clone(1) spawns git-remote-curl(1), which will be unable to\n> detect the repository due to a missing \"HEAD\".\n>\n> 3. git-clone(1) asks git-remote-curl(1) to list remote references.\n> This works just fine as this step does not require a local\n> repository\n>\n> 4. git-clone(1) creates the reference database as it has now learned\n> about the object format.\n\nSorry, but I am not sure I understand this step. I assume you mean\nby \"the object format\" what hash function is used to index the\nobjects (which can be learned from the remote \"origin\" in step 2 and\nwe can choose to use the one they use), not what ref backend is used\n(which is purely a local matter and we do not need to know what is\nused at the \"origin\"). Why do we need to wait initializing ref\nbackend until we learn what hash is being in use?\n\n> 5. git-clone(1) asks git-remote-curl(1) to fetch the remote packfile.\n> The latter notices that it doesn't have a repository available, but\n> it now knows to try and re-discover it.\n>\n> If the re-discovery succeeds in the last step we can continue with the\n> clone.\n\nOK.\n"},{"id":"485496","messageId":"xmqqil58z3vv.fsf@gitster.g","threadId":"60587","inReplyTo":"c7a9d6ef74ff39e660f80e2e104a96b7c875845d.1701863960.git.ps@pks.im","subject":"Re: [PATCH 4/7] builtin/clone: fix bundle URIs with mismatching object formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-08T23:11:16Z","receivedAt":"2023-12-08T23:11:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This is not a problem in the context of the files backend ...\n> It will become a problem though once\n> we land the reftable backend, which indeed does require to know about\n> the proper object format at the time of creation.\n\nOK. That answers the question I had on the previous step.\n\nThanks.\n"},{"id":"485509","messageId":"xmqq7clmn3w1.fsf@gitster.g","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-10T03:16:30Z","receivedAt":"2023-12-10T03:16:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While at it I noticed that this actually fixes a bug with bundle URIs\n> when the object formats diverge in this way.\n> ...\n> This patch series is actually the last incompatibility for the reftable\n> backend that I have found. All tests except for the files-backend\n> specific ones pass now with the current state I have at [1], which is\n> currently at e6f2f592b7 (t: skip tests which are incompatible with\n> reftable, 2023-11-24)\n\nAn existing test\n\n $ make && cd t && GIT_TEST_DEFAULT_HASH=sha256 sh t5550-http-fetch-dumb.sh\n\npasses with vanilla Git 2.43, but with these patches applied, it\nfails the \"7 - empty dumb HTTP\" step.\n\nThanks.\n\n"},{"id":"485533","messageId":"ZXbzzlyWC3HTUyDA@tanuki","threadId":"60587","inReplyTo":"xmqq7clmn3w1.fsf@gitster.g","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T11:34:38Z","receivedAt":"2023-12-11T11:34:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 09, 2023 at 07:16:30PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > While at it I noticed that this actually fixes a bug with bundle URIs\n> > when the object formats diverge in this way.\n> > ...\n> > This patch series is actually the last incompatibility for the reftable\n> > backend that I have found. All tests except for the files-backend\n> > specific ones pass now with the current state I have at [1], which is\n> > currently at e6f2f592b7 (t: skip tests which are incompatible with\n> > reftable, 2023-11-24)\n> \n> An existing test\n> \n> $ make && cd t && GIT_TEST_DEFAULT_HASH=sha256 sh t5550-http-fetch-dumb.sh\n> \n> passes with vanilla Git 2.43, but with these patches applied, it\n> fails the \"7 - empty dumb HTTP\" step.\n\nIndeed -- now that the GitLab CI definitions have landed in your master\nbranch I can also see this failure as part of our CI job [1]. And with\nthe NixOS httpd refactorings I'm actually able to run these tests in the\nfirst place. Really happy to see that things come together like this, as\nit means that we'll detect such issues before we send the series to the\nmailing list from now on.\n\nAnyway, regarding the test itself. I think the refactorings in this\npatch series uncover a preexisting and already-known issue with empty\nrepositories when using the dumb HTTP protocol: there is no proper way\nto know about the hash function that the remote repository uses [2].\n\nBefore my refactorings we used to fall back to the local default hash\nformat with which we've already initialized the repository, which is\nwrong. Now we use the hash format we detected via the remote, which we\ncannot detect because the remote is empty and does not advertise the\nhash function, so we fall back to SHA1 and thus also do the wrong thing.\nThe only correct thing here would be to use the actual hash function\nthat the remote repository uses, but we have no to do so. So we're kind\nof stuck here and can only choose between two different wrong ways to\npick the hash function.\n\nWe can restore the previous wrong behaviour by honoring GIT_DEFAULT_HASH\nin git-remote-curl(1) in the case where we do not have a repository set\nup yet. So something similar in spirit to the following:\n\n```\ndiff --git a/remote-curl.c b/remote-curl.c\nindex fc29757b65..7e97c9c2e9 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -27,6 +27,7 @@\n static struct remote *remote;\n /* always ends with a trailing slash */\n static struct strbuf url = STRBUF_INIT;\n+static int nongit;\n \n struct options {\n \tint verbosity;\n@@ -275,8 +276,30 @@ static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)\n {\n \tconst char *p = memchr(heads->buf, '\\t', heads->len);\n \tint algo;\n-\tif (!p)\n-\t\treturn the_hash_algo;\n+\n+\tif (!p) {\n+\t\tconst char *env;\n+\n+\t\tif (!nongit)\n+\t\t\treturn the_hash_algo;\n+\n+\t\t/*\n+\t\t * In case the remote neither advertises the hash format nor\n+\t\t * any references we have no way to detect the correct hash\n+\t\t * format. We can thus only guess what the remote is using,\n+\t\t * where the best guess is to fall back to the default hash.\n+\t\t */\n+\t\tenv = getenv(\"GIT_DEFAULT_HASH\");\n+\t\tif (env) {\n+\t\t\talgo = hash_algo_by_name(env);\n+\t\t\tif (algo == GIT_HASH_UNKNOWN)\n+\t\t\t\tdie(_(\"unknown hash algorithm '%s'\"), env);\n+\t\t} else {\n+\t\t\talgo = GIT_HASH_SHA1;\n+\t\t}\n+\n+\t\treturn &hash_algos[algo];\n+\t}\n \n \talgo = hash_algo_by_length((p - heads->buf) / 2);\n \tif (algo == GIT_HASH_UNKNOWN)\n@@ -1521,7 +1544,6 @@ static int stateless_connect(const char *service_name)\n int cmd_main(int argc, const char **argv)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint nongit;\n \tint ret = 1;\n \n \tsetup_git_directory_gently(&nongit);\n```\n\n+Cc brian, as he's the author of [2].\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/git/-/jobs/5723052108\n[2]: ac093d0790 (remote-curl: detect algorithm for dumb HTTP by size, 2020-06-19)\n"},{"id":"485534","messageId":"ZXbz2eQoz0ReP_IF@tanuki","threadId":"60587","inReplyTo":"xmqq1qbw1f0j.fsf@gitster.g","subject":"Re: [PATCH 1/7] setup: extract function to create the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T11:34:49Z","receivedAt":"2023-12-11T11:34:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 09, 2023 at 07:54:52AM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:\n> >> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n> >> > +static void create_reference_database(const char *initial_branch, int quiet)\n> >> > +{\n> >> > + struct strbuf err = STRBUF_INIT;\n> >> > + int reinit = is_reinit();\n> >> > +\n> >> > + /*\n> >> > + * We need to create a \"refs\" dir in any case so that older\n> >> > + * versions of git can tell that this is a repository.\n> >> > + */\n> >> \n> >> How does this work though, even if an earlier version of git can tell\n> >> that this is a repository, it still won't be able to read the reftable\n> >> backend. In that sense, what do we achieve here?\n> >\n> > This is a good question, and there is related ongoing discussion about\n> > this topic in the thread starting at [1]. There are a few benefits to\n> > letting clients discover such repos even if they don't understand the\n> > new reference backend format:\n> >\n> > - They know to stop walking up the parent-directory chain. Otherwise a\n> > client might end up detecting a Git repository in the parent dir.\n> >\n> > - The user gets a proper error message why the repository cannot be\n> > accessed. Instead of failing to detect the repository altogether we\n> > instead say that we don't understand the \"extensions.refFormat\"\n> > extension.\n> \n> Yup, both are very good reasons. Would it help to sneak a condensed\n> version of it in the in-code comment, perhaps?\n\nSure, let's do so. I failed to condense this meaningfully, but hope that\nthe result will be okay regardless of that.\n\nPatrick\n"},{"id":"485535","messageId":"ZXbz5KbvgrL5u1yW@tanuki","threadId":"60587","inReplyTo":"xmqqmsukz3yr.fsf@gitster.g","subject":"Re: [PATCH 3/7] remote-curl: rediscover repository when fetching refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T11:35:00Z","receivedAt":"2023-12-11T11:35:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Dec 09, 2023 at 08:09:32AM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > We're about to change git-clone(1) so that we set up the reference\n> > database at a later point. This change will cause git-remote-curl(1) to\n> > not detect the repository anymore due to \"HEAD\" not having been created\n> > yet at the time it spawns, and thus causes it to error out once it is\n> > asked to fetch the references.\n> >\n> > We can address this issue by trying to re-discover the Git repository in\n> > case none was detected at startup time. With this change, the clone will\n> > look as following:\n> >\n> > 1. git-clone(1) sets up the initial repository, excluding the\n> > reference database.\n> >\n> > 2. git-clone(1) spawns git-remote-curl(1), which will be unable to\n> > detect the repository due to a missing \"HEAD\".\n> >\n> > 3. git-clone(1) asks git-remote-curl(1) to list remote references.\n> > This works just fine as this step does not require a local\n> > repository\n> >\n> > 4. git-clone(1) creates the reference database as it has now learned\n> > about the object format.\n> \n> Sorry, but I am not sure I understand this step. I assume you mean\n> by \"the object format\" what hash function is used to index the\n> objects (which can be learned from the remote \"origin\" in step 2 and\n> we can choose to use the one they use), not what ref backend is used\n> (which is purely a local matter and we do not need to know what is\n> used at the \"origin\").\n\nYes, exactly. I'm never quite sure whether I should be saying \"hash\nfunction\" or \"object format\". I'll convert the message to say \"hash\nfunction\" instead to clarify.\n\n> Why do we need to wait initializing ref backend until we learn what\n> hash is being in use?\n\nThis is because of the reftable backend. With the files backend it never\nmattered much, because we do not encode the object format anywhere. But\nwith the reftable backend we do indeed encode the object format in the\ntables' header, so it's important to initialize it with the correct\nformat right from the start.\n\nI'll amend the commit message.\n\nPatrick\n"},{"id":"485537","messageId":"xmqqmsugvlbe.fsf@gitster.g","threadId":"60587","inReplyTo":"ZXbzzlyWC3HTUyDA@tanuki","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-11T14:57:25Z","receivedAt":"2023-12-11T14:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> An existing test\n>> \n>> $ make && cd t && GIT_TEST_DEFAULT_HASH=sha256 sh t5550-http-fetch-dumb.sh\n>> \n>> passes with vanilla Git 2.43, but with these patches applied, it\n>> fails the \"7 - empty dumb HTTP\" step.\n> ...\n> Before my refactorings we used to fall back to the local default hash\n> format with which we've already initialized the repository, which is\n> wrong. Now we use the hash format we detected via the remote, which we\n> cannot detect because the remote is empty and does not advertise the\n> hash function, so we fall back to SHA1 and thus also do the wrong thing.\n\nYeah, that is why I did *not* say \"the series *breaks* existing\ntests\". It triggers a failure, and in this case, a test failure\ndoes not mean the behaviour is broken because there is no correct\nanswer. ... oh, wait. There isn't?\n\nI wonder if the right thing to do is to advertise the hash function\neven from an absolutely empty repository. There are no objects in\nsuch a repository, but it should already know what hash function to\nuse when it creates its first object (determined at the repository\ncreation time), so that _could_ be advertised. \n\n> The only correct thing here would be to use the actual hash function\n> that the remote repository uses, but we have no to do so.\n\nWe have \"no way to do so\"? We have \"not done so\"?\n\nIt is possible for the client side to download the $GIT_DIR/config\nfile from the remote to learn what value extensions.objectFormat is\nin use over there instead, I think, but at the same time, I highly\nsuspect that dumb HTTP outlived its usefulness to warrant such an\nadditional investment of engineering resource.\n\nThe simplest \"fix\" might be to leave what happens in this narrow\ncase (i.e. cloning over dumb HTTP from an empty repository)\nundefined by not testing (or not insisting on one particular\noutcome), but ...\n\n> +Cc brian, as he's the author of [2].\n\n... of course I trust Brian more than I trust myself in this area ;-)\n\n> Patrick\n>\n> [1]: https://gitlab.com/gitlab-org/git/-/jobs/5723052108\n> [2]: ac093d0790 (remote-curl: detect algorithm for dumb HTTP by size, 2020-06-19)\n\nThanks.\n"},{"id":"485539","messageId":"ZXcrpGQhH121AQ7y@tanuki","threadId":"60587","inReplyTo":"xmqqmsugvlbe.fsf@gitster.g","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-11T15:32:52Z","receivedAt":"2023-12-11T15:32:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Dec 11, 2023 at 06:57:25AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> >> An existing test\n> >> \n> >> $ make && cd t && GIT_TEST_DEFAULT_HASH=sha256 sh t5550-http-fetch-dumb.sh\n> >> \n> >> passes with vanilla Git 2.43, but with these patches applied, it\n> >> fails the \"7 - empty dumb HTTP\" step.\n> > ...\n> > Before my refactorings we used to fall back to the local default hash\n> > format with which we've already initialized the repository, which is\n> > wrong. Now we use the hash format we detected via the remote, which we\n> > cannot detect because the remote is empty and does not advertise the\n> > hash function, so we fall back to SHA1 and thus also do the wrong thing.\n> \n> Yeah, that is why I did *not* say \"the series *breaks* existing\n> tests\". It triggers a failure, and in this case, a test failure\n> does not mean the behaviour is broken because there is no correct\n> answer. ... oh, wait. There isn't?\n> \n> I wonder if the right thing to do is to advertise the hash function\n> even from an absolutely empty repository. There are no objects in\n> such a repository, but it should already know what hash function to\n> use when it creates its first object (determined at the repository\n> creation time), so that _could_ be advertised. \n\nFor the smart HTTP and SSH protocols definitely, and we already do. But\nit's a different story for dumb HTTP, unfortunately, where there is no\nCGI-like thing sitting between the client and the repository's data.\n\n> > The only correct thing here would be to use the actual hash function\n> > that the remote repository uses, but we have no to do so.\n> \n> We have \"no way to do so\"? We have \"not done so\"?\n\nWe have not done so until now, and we have no easy way to change this on\nthe server-side as the server is not controlled by us in the first\nplace. That leaves two options I can think of:\n\n - Try harder on the client-side, e.g. by trying to download the\n gitconfig as you propose further down. I wonder whether admins would\n typically block access to the config, but doubt they do.\n\n - Change the format of `info/refs` to include the hash format, as this\n file _is_ controlled by us on the server-side. Doesn't help though\n in an empty repository, where the file is likely to never have been\n generated in the first place.\n\nSo it seems like downloading the gitconfig is the only viable option\nthat I can think of right now.\n\nIt does increase the potential attack surface though because we would\nstart to unconditionally parse a config file from an untrusted source,\nand we did hit issues in our config parser in the past already. You\ncould argue that we already parse untrusted configs via `.gitmodules`,\nbut these require opt-in to actually be used by anything if I'm not\nmistaken.\n\nSo... I dunno.\n\n> It is possible for the client side to download the $GIT_DIR/config\n> file from the remote to learn what value extensions.objectFormat is\n> in use over there instead, I think, but at the same time, I highly\n> suspect that dumb HTTP outlived its usefulness to warrant such an\n> additional investment of engineering resource.\n\nFair enough. All of this feels like an edge case (admin that uses dumb\nHTTP) in an edge case (the cloned repository uses SHA256) in an edge\ncase (the remote repository is empty). Sure, SHA256 is likely to gain in\npopularity eventually. But at the same time I'd expect that dumb HTTP\nwill become increasingly rare.\n\nTaken together, chances for this to happen should be fairly low.\n\n> The simplest \"fix\" might be to leave what happens in this narrow\n> case (i.e. cloning over dumb HTTP from an empty repository)\n> undefined by not testing (or not insisting on one particular\n> outcome), but ...\n\nI would be fine with that outcome, as well. It's not like the current\nbehaviour is correct in all cases either. The only benefit of that\nbehaviour is that a user can in fact work around the broken cases by\nsetting `GIT_HASH_DEFAULT` to the correct hash, and that benefit would\nbe retained by the diff I sent that made remote-curl.c aware of this\nenvironment variable.\n\nOne additional solution would be to print a user-visible warning a la\n\"warning: failed to detect hash function of empty remote repository\" and\nthen call it a day, potentially pointing out that a user can correct it\nby re-cloning with `GIT_HASH_DEFAULT`. But the warning may not be\nactionable by the user, because they may not know what hash function the\nremote uses, either.\n\nPatrick\n"},{"id":"485541","messageId":"CAOLa=ZRTQ3X7+PnNHaAeM5vKh=x3Vq9C_H1emUNzJB6PUBBwEA@mail.gmail.com","threadId":"60587","inReplyTo":"ZXFy0_T1AZLh058g@tanuki","subject":"Re: [PATCH 1/7] setup: extract function to create the refdb","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2023-12-11T15:50:17Z","receivedAt":"2023-12-11T15:50:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:\n>> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt wrote:\n>> > + /*\n>> > + * We need to create a \"refs\" dir in any case so that older\n>> > + * versions of git can tell that this is a repository.\n>> > + */\n>>\n>> How does this work though, even if an earlier version of git can tell\n>> that this is a repository, it still won't be able to read the reftable\n>> backend. In that sense, what do we achieve here?\n>\n> This is a good question, and there is related ongoing discussion about\n> this topic in the thread starting at [1]. There are a few benefits to\n> letting clients discover such repos even if they don't understand the\n> new reference backend format:\n>\n> - They know to stop walking up the parent-directory chain. Otherwise a\n> client might end up detecting a Git repository in the parent dir.\n>\n> - The user gets a proper error message why the repository cannot be\n> accessed. Instead of failing to detect the repository altogether we\n> instead say that we don't understand the \"extensions.refFormat\"\n> extension.\n>\n> Maybe there are other cases I can't think of right now.\n> [1]: \n\nThank Patrick, this does indeed make a lot of sense now. +1 that this\nwould be super useful as a comment here.\n"},{"id":"485545","messageId":"ZXeKldqyqN9ofTcF@tapette.crustytoothpaste.net","threadId":"60587","inReplyTo":"ZXcrpGQhH121AQ7y@tanuki","subject":"Re: [PATCH 0/7] clone: fix init of refdb with wrong object format","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-12-11T22:17:57Z","receivedAt":"2023-12-11T22:18:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-12-11 at 15:32:52, Patrick Steinhardt wrote:\n> On Mon, Dec 11, 2023 at 06:57:25AM -0800, Junio C Hamano wrote:\n> > We have \"no way to do so\"? We have \"not done so\"?\n> \n> We have not done so until now, and we have no easy way to change this on\n> the server-side as the server is not controlled by us in the first\n> place. That leaves two options I can think of:\n> \n> - Try harder on the client-side, e.g. by trying to download the\n> gitconfig as you propose further down. I wonder whether admins would\n> typically block access to the config, but doubt they do.\n> \n> - Change the format of `info/refs` to include the hash format, as this\n> file _is_ controlled by us on the server-side. Doesn't help though\n> in an empty repository, where the file is likely to never have been\n> generated in the first place.\n> \n> So it seems like downloading the gitconfig is the only viable option\n> that I can think of right now.\n\nI mean, we can add an `info/capabilities` file with capabilities and\nassume the repository is SHA-1 without it. I'm fine with that approach\nas well, and it can be implemented as part of `git update-server-info`\npretty easily.\n\nBut yes, absent that approach or parsing the config file, we'll have to\njust use the default settings.\n\n> > The simplest \"fix\" might be to leave what happens in this narrow\n> > case (i.e. cloning over dumb HTTP from an empty repository)\n> > undefined by not testing (or not insisting on one particular\n> > outcome), but ...\n> \n> I would be fine with that outcome, as well. It's not like the current\n> behaviour is correct in all cases either. The only benefit of that\n> behaviour is that a user can in fact work around the broken cases by\n> setting `GIT_HASH_DEFAULT` to the correct hash, and that benefit would\n> be retained by the diff I sent that made remote-curl.c aware of this\n> environment variable.\n\nThat would also be fine with me.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"485559","messageId":"cover.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1701863960.git.ps@pks.im","subject":"[PATCH v2 0/7] clone: fix init of refdb with wrong object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:37Z","receivedAt":"2023-12-12T07:00:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series to make git-clone(1)\ninitialize the reference database with the correct hash format. This is\na preparatory step for the reftable backend, which encodes the hash\nformat on-disk and thus requires us to get the hash format right at the\ntime of initialization.\n\nChanges compared to v1:\n\n - Patch 1: Extend the comment explaining why we always create the\n \"refs/\" directory.\n\n - Patch 3: Explain better why the patch is required in the first\n place.\n\n - Patch 4: Fix a typo.\n\n - Patch 7: Adapt a failing test to now assert the new behaviour.\n\nThanks for your reviews!\n\nPatrick\n\nPatrick Steinhardt (7):\n setup: extract function to create the refdb\n setup: allow skipping creation of the refdb\n remote-curl: rediscover repository when fetching refs\n builtin/clone: fix bundle URIs with mismatching object formats\n builtin/clone: set up sparse checkout later\n builtin/clone: skip reading HEAD when retrieving remote\n builtin/clone: create the refdb with the correct object format\n\n builtin/clone.c | 65 ++++++++++----------\n remote-curl.c | 7 ++-\n remote.c | 26 ++++----\n remote.h | 1 +\n setup.c | 114 ++++++++++++++++++++++--------------\n setup.h | 6 +-\n t/t5550-http-fetch-dumb.sh | 4 +-\n t/t5558-clone-bundle-uri.sh | 18 ++++++\n 8 files changed, 150 insertions(+), 91 deletions(-)\n\nRange-diff against v1:\n1: b69c57d272 ! 1: 2f34daa082 setup: extract function to create the refdb\n @@ Commit message\n calling `init_db()`. Extract the logic into a standalone function so\n that it becomes easier to do this refactoring.\n \n + While at it, expand the comment that explains why we always create the\n + \"refs/\" directory.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## setup.c ##\n @@ setup.c: void initialize_repository_version(int hash_algo, int reinit)\n +\tint reinit = is_reinit();\n +\n +\t/*\n -+\t * We need to create a \"refs\" dir in any case so that older\n -+\t * versions of git can tell that this is a repository.\n ++\t * We need to create a \"refs\" dir in any case so that older versions of\n ++\t * Git can tell that this is a repository. This serves two main purposes:\n ++\t *\n ++\t * - Clients will know to stop walking the parent-directory chain when\n ++\t * detecting the Git repository. Otherwise they may end up detecting\n ++\t * a Git repository in a parent directory instead.\n ++\t *\n ++\t * - Instead of failing to detect a repository with unknown reference\n ++\t * format altogether, old clients will print an error saying that\n ++\t * they do not understand the reference format extension.\n +\t */\n +\tsafe_create_dir(git_path(\"refs\"), 1);\n +\tadjust_shared_perm(git_path(\"refs\"));\n2: 090c52423e = 2: 40005ac1f1 setup: allow skipping creation of the refdb\n3: a1b86a0cbb ! 3: 374a1c514b remote-curl: rediscover repository when fetching refs\n @@ Metadata\n ## Commit message ##\n remote-curl: rediscover repository when fetching refs\n \n - We're about to change git-clone(1) so that we set up the reference\n - database at a later point. This change will cause git-remote-curl(1) to\n - not detect the repository anymore due to \"HEAD\" not having been created\n - yet at the time it spawns, and thus causes it to error out once it is\n - asked to fetch the references.\n + The reftable format encodes the hash function used by the repository\n + inside of its tables. The reftable backend thus needs to be initialized\n + with the correct hash function right from the start, or otherwise we may\n + end up writing tables with the wrong hash function. But git-clone(1)\n + initializes the reference database before learning about the hash\n + function used by the remote repository, which has never been a problem\n + with the reffiles backend.\n +\n + To fix this, we'll have to change git-clone(1) to be more careful and\n + only create the reference backend once it learned about the remote hash\n + function. This creates a problem for git-remote-curl(1), which will then\n + be spawned at a time where the repository is not yet fully-initialized.\n + Consequentially, git-remote-curl(1) will fail to detect the repository,\n + which eventually causes it to error out once it is asked to fetch remote\n + objects.\n \n We can address this issue by trying to re-discover the Git repository in\n case none was detected at startup time. With this change, the clone will\n @@ Commit message\n repository\n \n 4. git-clone(1) creates the reference database as it has now learned\n - about the object format.\n + about the hash function.\n \n 5. git-clone(1) asks git-remote-curl(1) to fetch the remote packfile.\n The latter notices that it doesn't have a repository available, but\n4: c7a9d6ef74 ! 4: 3bef564b57 builtin/clone: fix bundle URIs with mismatching object formats\n @@ Commit message\n is indeed not necessarily the case for the hash algorithm right now. So\n in practice it is the right thing to detect the remote's object format\n before downloading bundle URIs anyway, and not doing so causes clones\n - with bundle URIS to fail when the local default object format does not\n + with bundle URIs to fail when the local default object format does not\n match the remote repository's format.\n \n Unfortunately though, this creates a new issue: downloading bundles may\n5: 703ff77eea = 5: 917f15055f builtin/clone: set up sparse checkout later\n6: 6c919fb19c = 6: f3485a2708 builtin/clone: skip reading HEAD when retrieving remote\n7: eb5530e6a8 ! 7: f062b11550 builtin/clone: create the refdb with the correct object format\n @@ Commit message\n upcoming reftable backend when cloning repositories with the SHA256\n object format.\n \n + This change breaks a test in \"t5550-http-fetch-dumb.sh\" when cloning an\n + empty repository with `GIT_TEST_DEFAULT_HASH=sha256`. The test expects\n + the resulting hash format of the empty cloned repository to match the\n + default hash, but now we always end up with a sha1 repository. The\n + problem is that for dumb HTTP fetches, we have no easy way to figure out\n + the remote's hash function except for deriving it based on the hash\n + length of refs in `info/refs`. But as the remote repository is empty we\n + cannot rely on this detection mechanism.\n +\n + Before the change in this commit we already initialized the repository\n + with the default hash function and then left it as-is. With this patch\n + we always use the hash function detected via the remote, where we fall\n + back to \"sha1\" in case we cannot detect it.\n +\n + Neither the old nor the new behaviour are correct as we second-guess the\n + remote hash function in both cases. But given that this is a rather\n + unlikely edge case (we use the dumb HTTP protocol, the remote repository\n + uses SHA256 and the remote repository is empty), let's simply adapt the\n + test to assert the new behaviour. If we want to properly address this\n + edge case in the future we will have to extend the dumb HTTP protocol so\n + that we can properly detect the hash function for empty repositories.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## builtin/clone.c ##\n @@ setup.h: int init_db(const char *git_dir, const char *real_git_dir,\n \n /*\n * NOTE NOTE NOTE!!\n +\n + ## t/t5550-http-fetch-dumb.sh ##\n +@@ t/t5550-http-fetch-dumb.sh: test_expect_success 'create empty remote repository' '\n + \tsetup_post_update_server_info_hook \"$HTTPD_DOCUMENT_ROOT_PATH/empty.git\"\n + '\n + \n +-test_expect_success 'empty dumb HTTP repository has default hash algorithm' '\n ++test_expect_success 'empty dumb HTTP repository falls back to SHA1' '\n + \ttest_when_finished \"rm -fr clone-empty\" &&\n + \tgit clone $HTTPD_URL/dumb/empty.git clone-empty &&\n + \tgit -C clone-empty rev-parse --show-object-format >empty-format &&\n +-\ttest \"$(cat empty-format)\" = \"$(test_oid algo)\"\n ++\ttest \"$(cat empty-format)\" = sha1\n + '\n + \n + setup_askpass_helper\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \n2.43.GIT\n\n"},{"id":"485560","messageId":"2f34daa0823ccb5cfee355758ee00da22bb17c03.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 1/7] setup: extract function to create the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:41Z","receivedAt":"2023-12-12T07:00:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're about to let callers skip creation of the reference database when\ncalling `init_db()`. Extract the logic into a standalone function so\nthat it becomes easier to do this refactoring.\n\nWhile at it, expand the comment that explains why we always create the\n\"refs/\" directory.\n\nSigned-off-by: Patrick Steinhardt \n---\n setup.c | 103 +++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 65 insertions(+), 38 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex fc592dc6dd..865cfe6743 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1885,6 +1885,68 @@ void initialize_repository_version(int hash_algo, int reinit)\n \t\tgit_config_set_gently(\"extensions.objectformat\", NULL);\n }\n \n+static int is_reinit(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar junk[2];\n+\tint ret;\n+\n+\tgit_path_buf(&buf, \"HEAD\");\n+\tret = !access(buf.buf, R_OK) || readlink(buf.buf, junk, sizeof(junk) - 1) != -1;\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static void create_reference_database(const char *initial_branch, int quiet)\n+{\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint reinit = is_reinit();\n+\n+\t/*\n+\t * We need to create a \"refs\" dir in any case so that older versions of\n+\t * Git can tell that this is a repository. This serves two main purposes:\n+\t *\n+\t * - Clients will know to stop walking the parent-directory chain when\n+\t * detecting the Git repository. Otherwise they may end up detecting\n+\t * a Git repository in a parent directory instead.\n+\t *\n+\t * - Instead of failing to detect a repository with unknown reference\n+\t * format altogether, old clients will print an error saying that\n+\t * they do not understand the reference format extension.\n+\t */\n+\tsafe_create_dir(git_path(\"refs\"), 1);\n+\tadjust_shared_perm(git_path(\"refs\"));\n+\n+\tif (refs_init_db(&err))\n+\t\tdie(\"failed to set up refs db: %s\", err.buf);\n+\n+\t/*\n+\t * Point the HEAD symref to the initial branch with if HEAD does\n+\t * not yet exist.\n+\t */\n+\tif (!reinit) {\n+\t\tchar *ref;\n+\n+\t\tif (!initial_branch)\n+\t\t\tinitial_branch = git_default_branch_name(quiet);\n+\n+\t\tref = xstrfmt(\"refs/heads/%s\", initial_branch);\n+\t\tif (check_refname_format(ref, 0) < 0)\n+\t\t\tdie(_(\"invalid initial branch name: '%s'\"),\n+\t\t\t initial_branch);\n+\n+\t\tif (create_symref(\"HEAD\", ref, NULL) < 0)\n+\t\t\texit(1);\n+\t\tfree(ref);\n+\t}\n+\n+\tif (reinit && initial_branch)\n+\t\twarning(_(\"re-init: ignored --initial-branch=%s\"),\n+\t\t\tinitial_branch);\n+\n+\tstrbuf_release(&err);\n+}\n+\n static int create_default_files(const char *template_path,\n \t\t\t\tconst char *original_git_dir,\n \t\t\t\tconst char *initial_branch,\n@@ -1896,10 +1958,8 @@ static int create_default_files(const char *template_path,\n \tstruct stat st1;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *path;\n-\tchar junk[2];\n \tint reinit;\n \tint filemode;\n-\tstruct strbuf err = STRBUF_INIT;\n \tconst char *init_template_dir = NULL;\n \tconst char *work_tree = get_git_work_tree();\n \n@@ -1919,6 +1979,8 @@ static int create_default_files(const char *template_path,\n \treset_shared_repository();\n \tgit_config(git_default_config, NULL);\n \n+\treinit = is_reinit();\n+\n \t/*\n \t * We must make sure command-line options continue to override any\n \t * values we might have just re-read from the config.\n@@ -1962,39 +2024,7 @@ static int create_default_files(const char *template_path,\n \t\tadjust_shared_perm(get_git_dir());\n \t}\n \n-\t/*\n-\t * We need to create a \"refs\" dir in any case so that older\n-\t * versions of git can tell that this is a repository.\n-\t */\n-\tsafe_create_dir(git_path(\"refs\"), 1);\n-\tadjust_shared_perm(git_path(\"refs\"));\n-\n-\tif (refs_init_db(&err))\n-\t\tdie(\"failed to set up refs db: %s\", err.buf);\n-\n-\t/*\n-\t * Point the HEAD symref to the initial branch with if HEAD does\n-\t * not yet exist.\n-\t */\n-\tpath = git_path_buf(&buf, \"HEAD\");\n-\treinit = (!access(path, R_OK)\n-\t\t || readlink(path, junk, sizeof(junk)-1) != -1);\n-\tif (!reinit) {\n-\t\tchar *ref;\n-\n-\t\tif (!initial_branch)\n-\t\t\tinitial_branch = git_default_branch_name(quiet);\n-\n-\t\tref = xstrfmt(\"refs/heads/%s\", initial_branch);\n-\t\tif (check_refname_format(ref, 0) < 0)\n-\t\t\tdie(_(\"invalid initial branch name: '%s'\"),\n-\t\t\t initial_branch);\n-\n-\t\tif (create_symref(\"HEAD\", ref, NULL) < 0)\n-\t\t\texit(1);\n-\t\tfree(ref);\n-\t}\n-\n+\tcreate_reference_database(initial_branch, quiet);\n \tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n@@ -2158,9 +2188,6 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t\t\t prev_bare_repository,\n \t\t\t\t init_shared_repository,\n \t\t\t\t flags & INIT_DB_QUIET);\n-\tif (reinit && initial_branch)\n-\t\twarning(_(\"re-init: ignored --initial-branch=%s\"),\n-\t\t\tinitial_branch);\n \n \tcreate_object_directory();\n \n-- \n2.43.GIT\n\n"},{"id":"485561","messageId":"40005ac1f14c8d0c1fc474351554ebe4fdec1cf4.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 2/7] setup: allow skipping creation of the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:46Z","receivedAt":"2023-12-12T07:00:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Allow callers to skip creation of the reference database via a new flag\n`INIT_DB_SKIP_REFDB`, which is required for git-clone(1) so that we can\ncreate it at a later point once the object format has been discovered\nfrom the remote repository.\n\nNote that we also uplift the call to `create_reference_database()` into\n`init_db()`, which makes it easier to handle the new flag for us. This\nchanges the order in which we do initialization so that we now set up\nthe Git configuration before we create the reference database. In\npractice this move should not result in any change in behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n setup.c | 13 +++++--------\n setup.h | 5 +++--\n 2 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 865cfe6743..d6a1c59b7b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1949,11 +1949,9 @@ static void create_reference_database(const char *initial_branch, int quiet)\n \n static int create_default_files(const char *template_path,\n \t\t\t\tconst char *original_git_dir,\n-\t\t\t\tconst char *initial_branch,\n \t\t\t\tconst struct repository_format *fmt,\n \t\t\t\tint prev_bare_repository,\n-\t\t\t\tint init_shared_repository,\n-\t\t\t\tint quiet)\n+\t\t\t\tint init_shared_repository)\n {\n \tstruct stat st1;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -2024,7 +2022,6 @@ static int create_default_files(const char *template_path,\n \t\tadjust_shared_perm(get_git_dir());\n \t}\n \n-\tcreate_reference_database(initial_branch, quiet);\n \tinitialize_repository_version(fmt->hash_algo, 0);\n \n \t/* Check filemode trustability */\n@@ -2184,11 +2181,11 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \tvalidate_hash_algorithm(&repo_fmt, hash);\n \n \treinit = create_default_files(template_dir, original_git_dir,\n-\t\t\t\t initial_branch, &repo_fmt,\n-\t\t\t\t prev_bare_repository,\n-\t\t\t\t init_shared_repository,\n-\t\t\t\t flags & INIT_DB_QUIET);\n+\t\t\t\t &repo_fmt, prev_bare_repository,\n+\t\t\t\t init_shared_repository);\n \n+\tif (!(flags & INIT_DB_SKIP_REFDB))\n+\t\tcreate_reference_database(initial_branch, flags & INIT_DB_QUIET);\n \tcreate_object_directory();\n \n \tif (get_shared_repository()) {\ndiff --git a/setup.h b/setup.h\nindex b48cf1c43b..cbf538286b 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -169,8 +169,9 @@ int verify_repository_format(const struct repository_format *format,\n */\n void check_repository_format(struct repository_format *fmt);\n \n-#define INIT_DB_QUIET 0x0001\n-#define INIT_DB_EXIST_OK 0x0002\n+#define INIT_DB_QUIET (1 << 0)\n+#define INIT_DB_EXIST_OK (1 << 1)\n+#define INIT_DB_SKIP_REFDB (1 << 2)\n \n int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *template_dir, int hash_algo,\n-- \n2.43.GIT\n\n"},{"id":"485562","messageId":"374a1c514bcb6de4e1cfd7a3a664c014807598a2.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 3/7] remote-curl: rediscover repository when fetching refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:50Z","receivedAt":"2023-12-12T07:00:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable format encodes the hash function used by the repository\ninside of its tables. The reftable backend thus needs to be initialized\nwith the correct hash function right from the start, or otherwise we may\nend up writing tables with the wrong hash function. But git-clone(1)\ninitializes the reference database before learning about the hash\nfunction used by the remote repository, which has never been a problem\nwith the reffiles backend.\n\nTo fix this, we'll have to change git-clone(1) to be more careful and\nonly create the reference backend once it learned about the remote hash\nfunction. This creates a problem for git-remote-curl(1), which will then\nbe spawned at a time where the repository is not yet fully-initialized.\nConsequentially, git-remote-curl(1) will fail to detect the repository,\nwhich eventually causes it to error out once it is asked to fetch remote\nobjects.\n\nWe can address this issue by trying to re-discover the Git repository in\ncase none was detected at startup time. With this change, the clone will\nlook as following:\n\n 1. git-clone(1) sets up the initial repository, excluding the\n reference database.\n\n 2. git-clone(1) spawns git-remote-curl(1), which will be unable to\n detect the repository due to a missing \"HEAD\".\n\n 3. git-clone(1) asks git-remote-curl(1) to list remote references.\n This works just fine as this step does not require a local\n repository\n\n 4. git-clone(1) creates the reference database as it has now learned\n about the hash function.\n\n 5. git-clone(1) asks git-remote-curl(1) to fetch the remote packfile.\n The latter notices that it doesn't have a repository available, but\n it now knows to try and re-discover it.\n\nIf the re-discovery succeeds in the last step we can continue with the\nclone.\n\nSigned-off-by: Patrick Steinhardt \n---\n remote-curl.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ef05752ca5..fc29757b65 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1564,8 +1564,11 @@ int cmd_main(int argc, const char **argv)\n \t\tif (buf.len == 0)\n \t\t\tbreak;\n \t\tif (starts_with(buf.buf, \"fetch \")) {\n-\t\t\tif (nongit)\n-\t\t\t\tdie(_(\"remote-curl: fetch attempted without a local repo\"));\n+\t\t\tif (nongit) {\n+\t\t\t\tsetup_git_directory_gently(&nongit);\n+\t\t\t\tif (nongit)\n+\t\t\t\t\tdie(_(\"remote-curl: fetch attempted without a local repo\"));\n+\t\t\t}\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n-- \n2.43.GIT\n\n"},{"id":"485563","messageId":"3bef564b57ec67ecb54ae8f24396e48ad5967582.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 4/7] builtin/clone: fix bundle URIs with mismatching object formats","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:54Z","receivedAt":"2023-12-12T07:00:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We create the reference database in git-clone(1) quite early before\nconnecting to the remote repository. Given that we do not yet know about\nthe object format that the remote repository uses at that point in time\nthe consequence is that the refdb may be initialized with the wrong\nobject format.\n\nThis is not a problem in the context of the files backend as we do not\nencode the object format anywhere, and furthermore the only reference\nthat we write between initializing the refdb and learning about the\nobject format is the \"HEAD\" symref. It will become a problem though once\nwe land the reftable backend, which indeed does require to know about\nthe proper object format at the time of creation. We thus need to\nrearrange the logic in git-clone(1) so that we only initialize the refdb\nonce we have learned about the actual object format.\n\nAs a first step, move listing of remote references to happen earlier,\nwhich also allow us to set up the hash algorithm of the repository\nearlier now. While we aim to execute this logic as late as possible\nuntil after most of the setup has happened already, detection of the\nobject format and thus later the setup of the reference database must\nhappen before any other logic that may spawn Git commands or otherwise\nthese Git commands may not recognize the repository as such.\n\nThe first Git step where we expect the repository to be fully initalized\nis when we fetch bundles via bundle URIs. Funny enough, the comments\nthere also state that \"the_repository must match the cloned repo\", which\nis indeed not necessarily the case for the hash algorithm right now. So\nin practice it is the right thing to detect the remote's object format\nbefore downloading bundle URIs anyway, and not doing so causes clones\nwith bundle URIs to fail when the local default object format does not\nmatch the remote repository's format.\n\nUnfortunately though, this creates a new issue: downloading bundles may\ntake a long time, so if we list refs beforehand they might've grown\nstale meanwhile. It is not clear how to solve this issue except for a\nsecond reference listing though after we have downloaded the bundles,\nwhich may be an expensive thing to do.\n\nArguably though, it's preferable to have a staleness issue compared to\nbeing unable to clone a repository altogether.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 48 ++++++++++++++++++-------------------\n t/t5558-clone-bundle-uri.sh | 18 ++++++++++++++\n 2 files changed, 41 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c6357af949..d188650881 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1266,6 +1266,26 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (transport->smart_options && !deepen && !filter_options.choice)\n \t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \n+\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n+\trefspec_ref_prefixes(&remote->fetch,\n+\t\t\t &transport_ls_refs_options.ref_prefixes);\n+\tif (option_branch)\n+\t\texpand_ref_prefix(&transport_ls_refs_options.ref_prefixes,\n+\t\t\t\t option_branch);\n+\tif (!option_no_tags)\n+\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n+\t\t\t \"refs/tags/\");\n+\n+\trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n+\n+\t/*\n+\t * Now that we know what algorithm the remote side is using, let's set\n+\t * ours to the same thing.\n+\t */\n+\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n+\tinitialize_repository_version(hash_algo, 1);\n+\trepo_set_hash_algo(the_repository, hash_algo);\n+\n \t/*\n \t * Before fetching from the remote, download and install bundle\n \t * data from the --bundle-uri option.\n@@ -1281,24 +1301,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\tbundle_uri);\n \t\telse if (has_heuristic)\n \t\t\tgit_config_set_gently(\"fetch.bundleuri\", bundle_uri);\n-\t}\n-\n-\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n-\trefspec_ref_prefixes(&remote->fetch,\n-\t\t\t &transport_ls_refs_options.ref_prefixes);\n-\tif (option_branch)\n-\t\texpand_ref_prefix(&transport_ls_refs_options.ref_prefixes,\n-\t\t\t\t option_branch);\n-\tif (!option_no_tags)\n-\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes,\n-\t\t\t \"refs/tags/\");\n-\n-\trefs = transport_get_remote_refs(transport, &transport_ls_refs_options);\n-\n-\tif (refs)\n-\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n-\n-\tif (!bundle_uri) {\n+\t} else {\n \t\t/*\n \t\t* Populate transport->got_remote_bundle_uri and\n \t\t* transport->bundle_uri. We might get nothing.\n@@ -1319,13 +1322,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\t\t/*\n-\t\t * Now that we know what algorithm the remote side is using,\n-\t\t * let's set ours to the same thing.\n-\t\t */\n-\thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n-\tinitialize_repository_version(hash_algo, 1);\n-\trepo_set_hash_algo(the_repository, hash_algo);\n+\tif (refs)\n+\t\tmapped_refs = wanted_peer_refs(refs, &remote->fetch);\n \n \tif (mapped_refs) {\n \t\t/*\ndiff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh\nindex 996a08e90c..1ca5f745e7 100755\n--- a/t/t5558-clone-bundle-uri.sh\n+++ b/t/t5558-clone-bundle-uri.sh\n@@ -33,6 +33,15 @@ test_expect_success 'clone with path bundle' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone with path bundle and non-default hash' '\n+\ttest_when_finished \"rm -rf clone-path-non-default-hash\" &&\n+\tGIT_DEFAULT_HASH=sha256 git clone --bundle-uri=\"clone-from/B.bundle\" \\\n+\t\tclone-from clone-path-non-default-hash &&\n+\tgit -C clone-path-non-default-hash rev-parse refs/bundles/topic >actual &&\n+\tgit -C clone-from rev-parse topic >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone with file:// bundle' '\n \tgit clone --bundle-uri=\"file://$(pwd)/clone-from/B.bundle\" \\\n \t\tclone-from clone-file &&\n@@ -284,6 +293,15 @@ test_expect_success 'clone HTTP bundle' '\n \ttest_config -C clone-http log.excludedecoration refs/bundle/\n '\n \n+test_expect_success 'clone HTTP bundle with non-default hash' '\n+\ttest_when_finished \"rm -rf clone-http-non-default-hash\" &&\n+\tGIT_DEFAULT_HASH=sha256 git clone --bundle-uri=\"$HTTPD_URL/B.bundle\" \\\n+\t\t\"$HTTPD_URL/smart/fetch.git\" clone-http-non-default-hash &&\n+\tgit -C clone-http-non-default-hash rev-parse refs/bundles/topic >actual &&\n+\tgit -C clone-from rev-parse topic >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone bundle list (HTTP, no heuristic)' '\n \ttest_when_finished rm -f trace*.txt &&\n \n-- \n2.43.GIT\n\n"},{"id":"485564","messageId":"917f15055feba4847927edced71454f0faa3da7c.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 5/7] builtin/clone: set up sparse checkout later","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:00:59Z","receivedAt":"2023-12-12T07:01:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When asked to do a sparse checkout, then git-clone(1) will spawn\n`git sparse-checkout set` to set up the configuration accordingly. This\nrequires a proper Git repository or otherwise the command will fail. But\nas we are about to move creation of the reference database to a later\npoint, this prerequisite will not hold anymore.\n\nMove the logic to a later point in time where we know to have created\nthe reference database already.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex d188650881..9c60923f31 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1185,9 +1185,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n-\t\treturn 1;\n-\n \tremote = remote_get(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n@@ -1426,6 +1423,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdissociate_from_references();\n \t}\n \n+\tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n+\t\treturn 1;\n+\n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout(submodule_progress, filter_submodules);\n \n-- \n2.43.GIT\n\n"},{"id":"485565","messageId":"f3485a2708a12dc303308b8150d10d3195b04918.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 6/7] builtin/clone: skip reading HEAD when retrieving remote","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:01:03Z","receivedAt":"2023-12-12T07:01:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"After we have set up the remote configuration in git-clone(1) we'll call\n`remote_get()` to read the remote from the on-disk configuration. But\nnext to reading the on-disk configuration, `remote_get()` will also\ncause us to try and read the repository's HEAD reference so that we can\nfigure out the current branch. Besides being pointless in git-clone(1)\nbecause we're operating in an empty repository anyway, this will also\nbreak once we move creation of the reference database to a later point\nin time.\n\nRefactor the code to introduce a new `remote_get_early()` function that\nwill skip reading the HEAD reference to address this issue.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 2 +-\n remote.c | 26 ++++++++++++++++----------\n remote.h | 1 +\n 3 files changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9c60923f31..06966c5d4c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1185,7 +1185,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_required_reference.nr || option_optional_reference.nr)\n \t\tsetup_reference();\n \n-\tremote = remote_get(remote_name);\n+\tremote = remote_get_early(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n \t\t\tbranch_top.buf);\ndiff --git a/remote.c b/remote.c\nindex abb24822be..051d0a64a0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -509,7 +509,7 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t}\n }\n \n-static void read_config(struct repository *repo)\n+static void read_config(struct repository *repo, int early)\n {\n \tint flag;\n \n@@ -518,7 +518,7 @@ static void read_config(struct repository *repo)\n \trepo->remote_state->initialized = 1;\n \n \trepo->remote_state->current_branch = NULL;\n-\tif (startup_info->have_repository) {\n+\tif (startup_info->have_repository && !early) {\n \t\tconst char *head_ref = refs_resolve_ref_unsafe(\n \t\t\tget_main_ref_store(repo), \"HEAD\", 0, NULL, &flag);\n \t\tif (head_ref && (flag & REF_ISSYMREF) &&\n@@ -561,7 +561,7 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,\n \n const char *remote_for_branch(struct branch *branch, int *explicit)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \treturn remotes_remote_for_branch(the_repository->remote_state, branch,\n@@ -587,7 +587,7 @@ remotes_pushremote_for_branch(struct remote_state *remote_state,\n \n const char *pushremote_for_branch(struct branch *branch, int *explicit)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \treturn remotes_pushremote_for_branch(the_repository->remote_state,\n@@ -599,7 +599,7 @@ static struct remote *remotes_remote_get(struct remote_state *remote_state,\n \n const char *remote_ref_for_branch(struct branch *branch, int for_push)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \tif (branch) {\n@@ -709,7 +709,13 @@ remotes_remote_get(struct remote_state *remote_state, const char *name)\n \n struct remote *remote_get(const char *name)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n+\treturn remotes_remote_get(the_repository->remote_state, name);\n+}\n+\n+struct remote *remote_get_early(const char *name)\n+{\n+\tread_config(the_repository, 1);\n \treturn remotes_remote_get(the_repository->remote_state, name);\n }\n \n@@ -722,7 +728,7 @@ remotes_pushremote_get(struct remote_state *remote_state, const char *name)\n \n struct remote *pushremote_get(const char *name)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \treturn remotes_pushremote_get(the_repository->remote_state, name);\n }\n \n@@ -738,7 +744,7 @@ int remote_is_configured(struct remote *remote, int in_repo)\n int for_each_remote(each_remote_fn fn, void *priv)\n {\n \tint i, result = 0;\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tfor (i = 0; i < the_repository->remote_state->remotes_nr && !result;\n \t i++) {\n \t\tstruct remote *remote =\n@@ -1831,7 +1837,7 @@ struct branch *branch_get(const char *name)\n {\n \tstruct branch *ret;\n \n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tif (!name || !*name || !strcmp(name, \"HEAD\"))\n \t\tret = the_repository->remote_state->current_branch;\n \telse\n@@ -1973,7 +1979,7 @@ static const char *branch_get_push_1(struct remote_state *remote_state,\n \n const char *branch_get_push(struct branch *branch, struct strbuf *err)\n {\n-\tread_config(the_repository);\n+\tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \tif (!branch)\ndiff --git a/remote.h b/remote.h\nindex cdc8b1db42..79353ba226 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -118,6 +118,7 @@ struct remote {\n * and configuration.\n */\n struct remote *remote_get(const char *name);\n+struct remote *remote_get_early(const char *name);\n \n struct remote *pushremote_get(const char *name);\n int remote_is_configured(struct remote *remote, int in_repo);\n-- \n2.43.GIT\n\n"},{"id":"485566","messageId":"f062b11550a559fb0c96ef0fa188110f6116e3c6.1702361370.git.ps@pks.im","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"[PATCH v2 7/7] builtin/clone: create the refdb with the correct object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:01:07Z","receivedAt":"2023-12-12T07:01:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're currently creating the reference database with a potentially\nincorrect object format when the remote repository's object format is\ndifferent from the local default object format. This works just fine for\nnow because the files backend never records the object format anywhere.\nBut this logic will fail with any new reference backend that encodes\nthis information in some form either on-disk or in-memory.\n\nThe preceding commits have reshuffled code in git-clone(1) so that there\nis no code path that will access the reference database before we have\ndetected the remote's object format. With these refactorings we can now\ndefer initialization of the reference database until after we have\nlearned the remote's object format and thus initialize it with the\ncorrect format from the get-go.\n\nThese refactorings are required to make git-clone(1) work with the\nupcoming reftable backend when cloning repositories with the SHA256\nobject format.\n\nThis change breaks a test in \"t5550-http-fetch-dumb.sh\" when cloning an\nempty repository with `GIT_TEST_DEFAULT_HASH=sha256`. The test expects\nthe resulting hash format of the empty cloned repository to match the\ndefault hash, but now we always end up with a sha1 repository. The\nproblem is that for dumb HTTP fetches, we have no easy way to figure out\nthe remote's hash function except for deriving it based on the hash\nlength of refs in `info/refs`. But as the remote repository is empty we\ncannot rely on this detection mechanism.\n\nBefore the change in this commit we already initialized the repository\nwith the default hash function and then left it as-is. With this patch\nwe always use the hash function detected via the remote, where we fall\nback to \"sha1\" in case we cannot detect it.\n\nNeither the old nor the new behaviour are correct as we second-guess the\nremote hash function in both cases. But given that this is a rather\nunlikely edge case (we use the dumb HTTP protocol, the remote repository\nuses SHA256 and the remote repository is empty), let's simply adapt the\ntest to assert the new behaviour. If we want to properly address this\nedge case in the future we will have to extend the dumb HTTP protocol so\nthat we can properly detect the hash function for empty repositories.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 9 ++++++++-\n setup.c | 2 +-\n setup.h | 1 +\n t/t5550-http-fetch-dumb.sh | 4 ++--\n 4 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 06966c5d4c..fd052b8b54 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1097,8 +1097,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\t/*\n+\t * Initialize the repository, but skip initializing the reference\n+\t * database. We do not yet know about the object format of the\n+\t * repository, and reference backends may persist that information into\n+\t * their on-disk data structures.\n+\t */\n \tinit_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,\n-\t\tdo_not_override_repo_unix_permissions, INIT_DB_QUIET);\n+\t\tdo_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);\n \n \tif (real_git_dir) {\n \t\tfree((char *)git_dir);\n@@ -1282,6 +1288,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \thash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));\n \tinitialize_repository_version(hash_algo, 1);\n \trepo_set_hash_algo(the_repository, hash_algo);\n+\tcreate_reference_database(NULL, 1);\n \n \t/*\n \t * Before fetching from the remote, download and install bundle\ndiff --git a/setup.c b/setup.c\nindex d6a1c59b7b..155fe13f70 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1897,7 +1897,7 @@ static int is_reinit(void)\n \treturn ret;\n }\n \n-static void create_reference_database(const char *initial_branch, int quiet)\n+void create_reference_database(const char *initial_branch, int quiet)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tint reinit = is_reinit();\ndiff --git a/setup.h b/setup.h\nindex cbf538286b..3f0f17c351 100644\n--- a/setup.h\n+++ b/setup.h\n@@ -178,6 +178,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t const char *initial_branch, int init_shared_repository,\n \t unsigned int flags);\n void initialize_repository_version(int hash_algo, int reinit);\n+void create_reference_database(const char *initial_branch, int quiet);\n \n /*\n * NOTE NOTE NOTE!!\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex e444b30bf6..4c3b32785d 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -66,11 +66,11 @@ test_expect_success 'create empty remote repository' '\n \tsetup_post_update_server_info_hook \"$HTTPD_DOCUMENT_ROOT_PATH/empty.git\"\n '\n \n-test_expect_success 'empty dumb HTTP repository has default hash algorithm' '\n+test_expect_success 'empty dumb HTTP repository falls back to SHA1' '\n \ttest_when_finished \"rm -fr clone-empty\" &&\n \tgit clone $HTTPD_URL/dumb/empty.git clone-empty &&\n \tgit -C clone-empty rev-parse --show-object-format >empty-format &&\n-\ttest \"$(cat empty-format)\" = \"$(test_oid algo)\"\n+\ttest \"$(cat empty-format)\" = sha1\n '\n \n setup_askpass_helper\n-- \n2.43.GIT\n\n"},{"id":"485587","messageId":"xmqqfs07rzw2.fsf@gitster.g","threadId":"60587","inReplyTo":"cover.1702361370.git.ps@pks.im","subject":"Re: [PATCH v2 0/7] clone: fix init of refdb with wrong object format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T19:20:45Z","receivedAt":"2023-12-12T19:20:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is the second version of my patch series to make git-clone(1)\n> initialize the reference database with the correct hash format. This is\n> a preparatory step for the reftable backend, which encodes the hash\n> format on-disk and thus requires us to get the hash format right at the\n> time of initialization.\n>\n> Changes compared to v1:\n>\n> - Patch 1: Extend the comment explaining why we always create the\n> \"refs/\" directory.\n>\n> - Patch 3: Explain better why the patch is required in the first\n> place.\n>\n> - Patch 4: Fix a typo.\n>\n> - Patch 7: Adapt a failing test to now assert the new behaviour.\n>\n> Thanks for your reviews!\n\nEverything I see in the range-diff looks very sensible.\n\nWill replace. Thanks.\n"}]}