{"thread":{"id":"61016","subject":"[PATCH 0/2] builtin/clone: allow remote helpers to detect repo","startedAt":"2024-02-27T14:27:39Z","lastAt":"2024-05-03T02:23:22Z","messageCount":18,"participants":["Patrick Steinhardt","Junio C Hamano","Karthik Nayak","Justin Tobler","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"489510","messageId":"cover.1709041721.git.ps@pks.im","threadId":"61016","inReplyTo":null,"subject":"[PATCH 0/2] builtin/clone: allow remote helpers to detect repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-27T14:27:34Z","receivedAt":"2024-02-27T14:27:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series addresses a regression reported by Mike in Git v2.44\nwhere remote helpers cannot access the Git repository anymore when\nrunning git-clone(1).\n\nThe root cause of this is that we have started to initialize the refdb\nat a later point, after the helper is spawned. This is required such\nthat we can initialize it with the correct object format, which fixes\nclones of SHA256 repositories with the reftable format and when using\nbundles.\n\nThe proposed fix here is to partially initialize the refdb with just\nenough data such that it can be discovered. The fix isn't pretty, but\naddresses the issue. I also couldn't come up with a better idea than\nthis.\n\nPatrick\n\nPatrick Steinhardt (2):\n refs/reftable: don't fail empty transactions in repo without HEAD\n builtin/clone: allow remote helpers to detect repo\n\n builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n refs/reftable-backend.c | 1 +\n setup.c | 9 +++++++-\n t/t0610-reftable-basics.sh | 13 +++++++++++\n t/t5801/git-remote-testgit | 5 +++++\n 5 files changed, 73 insertions(+), 1 deletion(-)\n\n\nbase-commit: a2082dbdd315aa4dd3f315545e5b3ab3b3e2d894\n-- \n2.44.0\n\n"},{"id":"489511","messageId":"95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im","threadId":"61016","inReplyTo":"cover.1709041721.git.ps@pks.im","subject":"[PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-27T14:27:39Z","receivedAt":"2024-02-27T14:27:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Under normal circumstances, it shouldn't ever happen that a repository\nhas no HEAD reference. In fact, git-update-ref(1) would fail any request\nto delete the HEAD reference, and a newly initialized repository always\npre-creates it, too.\n\nBut in the next commit, we are going to change git-clone(1) to partially\ninitialize the refdb just up to the point where remote helpers can find\nthe repository. With that change, we are going to run into a situation\nwhere repositories have no refs at all.\n\nNow there is a very particular edge case in this situation: when\npreparing an empty ref transacton, we end up returning whatever value\n`read_ref_without_reload()` returned to the caller. Under normal\nconditions this would be fine: \"HEAD\" should usually exist, and thus the\nfunction would return `0`. But if \"HEAD\" doesn't exist, the function\nreturns a positive value which we end up returning to the caller.\n\nFix this bug by resetting the return code to `0` and add a test.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 1 +\n t/t0610-reftable-basics.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex a14f2ad7f4..45568818f0 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t &head_referent, &head_type);\n \tif (ret < 0)\n \t\tgoto done;\n+\tret = 0;\n \n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *u = transaction->updates[i];\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 6a131e40b8..c5f4d23433 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -328,6 +328,19 @@ test_expect_success 'ref transaction: writes are synced' '\n \tEOF\n '\n \n+test_expect_success 'ref transaction: empty transaction in empty repo' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\ttest_commit -C repo --no-tag A &&\n+\tCOMMIT=$(git -C repo rev-parse HEAD) &&\n+\tgit -C repo update-ref -d refs/heads/main &&\n+\ttest-tool -C repo ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n+\tgit -C repo update-ref --stdin <<-EOF\n+\tprepare\n+\tcommit\n+\tEOF\n+'\n+\n test_expect_success 'pack-refs: compacts tables' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n-- \n2.44.0\n\n"},{"id":"489512","messageId":"9d888adf92e9a8af7c18847219f97d3e595e3e36.1709041721.git.ps@pks.im","threadId":"61016","inReplyTo":"cover.1709041721.git.ps@pks.im","subject":"[PATCH 2/2] builtin/clone: allow remote helpers to detect repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-27T14:27:44Z","receivedAt":"2024-02-27T14:27:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 18c9cb7524 (builtin/clone: create the refdb with the correct object\nformat, 2023-12-12), we have changed git-clone(1) so that it delays\ncreation of the refdb until after it has learned about the remote's\nobject format. This change was required for the reftable backend, which\nencodes the object format into the tables. So if we pre-initialized the\nrefdb with the default object format, but the remote uses a different\nobject format than that, then the resulting tables would have encoded\nthe wrong object format.\n\nThis change unfortunately breaks remote helpers which try to access the\nrepository that is about to be created. Because the refdb has not yet\nbeen initialized at the point where we spawn the remote helper, we also\ndon't yet have \"HEAD\" or \"refs/\". Consequently, any Git commands ran by\nthe remote helper which try to access the repository would fail because\nit cannot be discovered.\n\nThis is essentially a chicken-and-egg problem: we cannot initialize the\nrefdb because we don't know about the object format. But we cannot learn\nabout the object format because the remote helper may be unable to\naccess the partially-initialized repository.\n\nIdeally, we would address this issue via capabilities. But the remote\nhelper protocol is not structured in a way that guarantees that the\ncapability announcement happens before the remote helper tries to access\nthe repository.\n\nInstead, fix this issue by partially initializing the refdb up to the\npoint where it becomes discoverable by Git commands.\n\nReported-by: Mike Hommey \nSigned-off-by: Patrick Steinhardt \n---\n builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n setup.c | 9 +++++++-\n t/t5801/git-remote-testgit | 5 +++++\n 3 files changed, 59 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bad1b70ce8..5d7f112125 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -926,6 +926,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct ref *mapped_refs = NULL;\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n@@ -1125,6 +1126,50 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_dir = real_git_dir;\n \t}\n \n+\t/*\n+\t * We have a chicken-and-egg situation between initializing the refdb\n+\t * and spawning transport helpers:\n+\t *\n+\t * - Initializing the refdb requires us to know about the object\n+\t * format. We thus have to spawn the transport helper to learn\n+\t * about it.\n+\t *\n+\t * - The transport helper may want to access the Git repository. But\n+\t * because the refdb has not been initialized, we don't have \"HEAD\"\n+\t * or \"refs/\". Thus, the helper cannot find the Git repository.\n+\t *\n+\t * Ideally, we would have structured the helper protocol such that it's\n+\t * mandatory for the helper to first announce its capabilities without\n+\t * yet assuming a fully initialized repository. Like that, we could\n+\t * have added a \"lazy-refdb-init\" capability that announces whether the\n+\t * helper is ready to handle not-yet-initialized refdbs. If any helper\n+\t * didn't support them, we would have fully initialized the refdb with\n+\t * the SHA1 object format, but later on bailed out if we found out that\n+\t * the remote repository used a different object format.\n+\t *\n+\t * But we didn't, and thus we use the following workaround to partially\n+\t * initialize the repository's refdb such that it can be discovered by\n+\t * Git commands. To do so, we:\n+\t *\n+\t * - Create an invalid HEAD ref pointing at \"refs/heads/.invalid\".\n+\t *\n+\t * - Create the \"refs/\" directory.\n+\t *\n+\t * - Set up the ref storage format and repository version as\n+\t * required.\n+\t *\n+\t * This is sufficient for Git commands to discover the Git directory.\n+\t */\n+\tinitialize_repository_version(GIT_HASH_UNKNOWN,\n+\t\t\t\t the_repository->ref_storage_format, 1);\n+\n+\tstrbuf_addf(&buf, \"%s/HEAD\", git_dir);\n+\twrite_file(buf.buf, \"ref: refs/heads/.invalid\");\n+\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, \"%s/refs\", git_dir);\n+\tsafe_create_dir(buf.buf, 1);\n+\n \t/*\n \t * additional config can be injected with -c, make sure it's included\n \t * after init_db, which clears the entire config environment.\n@@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n+\tstrbuf_release(&buf);\n \tstrbuf_release(&key);\n \tfree_refs(mapped_refs);\n \tfree_refs(remote_head_points_at);\ndiff --git a/setup.c b/setup.c\nindex b69b1cbc2a..e3b76e84b5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,\n \tchar repo_version_string[10];\n \tint repo_version = GIT_REPO_VERSION;\n \n+\t/*\n+\t * Note that we initialize the repository version to 1 when the ref\n+\t * storage format is unknown. This is on purpose so that we can add the\n+\t * correct object format to the config during git-clone(1). The format\n+\t * version will get adjusted by git-clone(1) once it has learned about\n+\t * the remote repository's format.\n+\t */\n \tif (hash_algo != GIT_HASH_SHA1 ||\n \t ref_storage_format != REF_STORAGE_FORMAT_FILES)\n \t\trepo_version = GIT_REPO_VERSION_READ;\n@@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,\n \t\t \"%d\", repo_version);\n \tgit_config_set(\"core.repositoryformatversion\", repo_version_string);\n \n-\tif (hash_algo != GIT_HASH_SHA1)\n+\tif (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)\n \t\tgit_config_set(\"extensions.objectformat\",\n \t\t\t hash_algos[hash_algo].name);\n \telse if (reinit)\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex 1544d6dc6b..bcfb358c51 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -12,6 +12,11 @@ url=$2\n \n dir=\"$GIT_DIR/testgit/$alias\"\n \n+if ! git rev-parse --is-inside-git-dir\n+then\n+\texit 1\n+fi\n+\n h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n \n-- \n2.44.0\n\n"},{"id":"489573","messageId":"xmqqcyshr5em.fsf@gitster.g","threadId":"61016","inReplyTo":"cover.1709041721.git.ps@pks.im","subject":"Re: [PATCH 0/2] builtin/clone: allow remote helpers to detect repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T20:58:09Z","receivedAt":"2024-02-27T20:58:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this patch series addresses a regression reported by Mike in Git v2.44\n> where remote helpers cannot access the Git repository anymore when\n> running git-clone(1).\n> ...\n> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n> refs/reftable-backend.c | 1 +\n\nSorry, but this confuses me. Was a regression really in v2.44.0,\nwhere refs/reftable-backend.c did not even exist? If so why does a\nfix for it need to touch that file?\n\nThanks.\n"},{"id":"489578","messageId":"xmqqle75ppa3.fsf@gitster.g","threadId":"61016","inReplyTo":"9d888adf92e9a8af7c18847219f97d3e595e3e36.1709041721.git.ps@pks.im","subject":"Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T21:31:48Z","receivedAt":"2024-02-27T21:31:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Instead, fix this issue by partially initializing the refdb up to the\n> point where it becomes discoverable by Git commands.\n\nAs you mentioned earlier, this indeed is ugly, but I agree that\nthere is no other reasonable way out. I am also unsure if this is a\nviable workaround in the longer term, but it should do for now.\n\n> +\t/*\n> +\t * We have a chicken-and-egg situation between initializing the refdb\n> +\t * and spawning transport helpers:\n> +\t *\n> +\t * - Initializing the refdb requires us to know about the object\n> +\t * format. We thus have to spawn the transport helper to learn\n> +\t * about it.\n> +\t * - The transport helper may want to access the Git repository. But\n> +\t * because the refdb has not been initialized, we don't have \"HEAD\"\n> +\t * or \"refs/\". Thus, the helper cannot find the Git repository.\n> +\t *\n> +\t * Ideally, we would have structured the helper protocol such that it's\n> +\t * mandatory for the helper to first announce its capabilities without\n> +\t * yet assuming a fully initialized repository. Like that, we could\n> +\t * have added a \"lazy-refdb-init\" capability that announces whether the\n> +\t * helper is ready to handle not-yet-initialized refdbs. If any helper\n> +\t * didn't support them, we would have fully initialized the refdb with\n> +\t * the SHA1 object format, but later on bailed out if we found out that\n> +\t * the remote repository used a different object format.\n> +\t * But we didn't, and thus we use the following workaround to partially\n> +\t * initialize the repository's refdb such that it can be discovered by\n> +\t * Git commands. To do so, we:\n> +\t *\n> +\t * - Create an invalid HEAD ref pointing at \"refs/heads/.invalid\".\n> +\t *\n> +\t * - Create the \"refs/\" directory.\n> +\t *\n> +\t * - Set up the ref storage format and repository version as\n> +\t * required.\n\n\"as required\" by whom and in what way? \n\n\"The code to recognize a repository requires them to be set already,\nbut they do not have to be the real value---we just assign random\nvalid values for now, let remote helper do its work and then set the\nreal values after they are done\" would be a plausible interpretation\nof the above. Is that what is going on?\n\n> +\t * This is sufficient for Git commands to discover the Git directory.\n> +\t */\n> +\tinitialize_repository_version(GIT_HASH_UNKNOWN,\n> +\t\t\t\t the_repository->ref_storage_format, 1);\n\nOK, so we start with \"we do not know it yet\" here.\n\n> +\tstrbuf_addf(&buf, \"%s/HEAD\", git_dir);\n> +\twrite_file(buf.buf, \"ref: refs/heads/.invalid\");\n> +\n> +\tstrbuf_reset(&buf);\n> +\tstrbuf_addf(&buf, \"%s/refs\", git_dir);\n> +\tsafe_create_dir(buf.buf, 1);\n> +\n> \t/*\n> \t * additional config can be injected with -c, make sure it's included\n> \t * after init_db, which clears the entire config environment.\n> @@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tfree(remote_name);\n> \tstrbuf_release(&reflog_msg);\n> \tstrbuf_release(&branch_top);\n> +\tstrbuf_release(&buf);\n> \tstrbuf_release(&key);\n> \tfree_refs(mapped_refs);\n> \tfree_refs(remote_head_points_at);\n> diff --git a/setup.c b/setup.c\n> index b69b1cbc2a..e3b76e84b5 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,\n> \tchar repo_version_string[10];\n> \tint repo_version = GIT_REPO_VERSION;\n> \n> +\t/*\n> +\t * Note that we initialize the repository version to 1 when the ref\n> +\t * storage format is unknown. This is on purpose so that we can add the\n> +\t * correct object format to the config during git-clone(1). The format\n> +\t * version will get adjusted by git-clone(1) once it has learned about\n> +\t * the remote repository's format.\n> +\t */\n> \tif (hash_algo != GIT_HASH_SHA1 ||\n> \t ref_storage_format != REF_STORAGE_FORMAT_FILES)\n> \t\trepo_version = GIT_REPO_VERSION_READ;\n> @@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,\n> \t\t \"%d\", repo_version);\n> \tgit_config_set(\"core.repositoryformatversion\", repo_version_string);\n> \n> -\tif (hash_algo != GIT_HASH_SHA1)\n> +\tif (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)\n> \t\tgit_config_set(\"extensions.objectformat\",\n> \t\t\t hash_algos[hash_algo].name);\n> \telse if (reinit)\n\nHere we refrain from writing extensions.objectformat in the\nrepository when we do not know what hash function is being used.\nWithout this change, we would instead remove extensions.objectformat,\nwhich does not sound too bad, as \"reinit\" is passed by the new call\nwe saw above and this \"else if (reinit)\" will do exactly that anyway.\n\nIn any case, after we finish talking with the other side over the\ntransport, we make another call to initialize_repository_version()\nwith the real objectformat and everything will be fine.\n\nThe ealier description of the implementation idea made it sound\nreally yucky, but I do not think the solution presented here is too\nbad.\n\n> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> index 1544d6dc6b..bcfb358c51 100755\n> --- a/t/t5801/git-remote-testgit\n> +++ b/t/t5801/git-remote-testgit\n> @@ -12,6 +12,11 @@ url=$2\n> \n> dir=\"$GIT_DIR/testgit/$alias\"\n> \n> +if ! git rev-parse --is-inside-git-dir\n> +then\n> +\texit 1\n> +fi\n> +\n> h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n> t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n"},{"id":"489579","messageId":"xmqqh6htpp6k.fsf@gitster.g","threadId":"61016","inReplyTo":"xmqqcyshr5em.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/clone: allow remote helpers to detect repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T21:33:55Z","receivedAt":"2024-02-27T21:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\n>\n>> Hi,\n>>\n>> this patch series addresses a regression reported by Mike in Git v2.44\n>> where remote helpers cannot access the Git repository anymore when\n>> running git-clone(1).\n>> ...\n>> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n>> refs/reftable-backend.c | 1 +\n>\n> Sorry, but this confuses me. Was a regression really in v2.44.0,\n> where refs/reftable-backend.c did not even exist? If so why does a\n> fix for it need to touch that file?\n>\n> Thanks.\n\nI guess [2/2] alone is the fix to be applied directly on top of v2.44.0\nand eventually be merged to 'maint' to become v2.44.1 release, while\n[1/2] is necessary to adjust reftable backend when such a fix is\nmerged to more recent codebase that already has the reftable\nbackend?\n\n"},{"id":"489609","messageId":"CAOLa=ZSycN0iYbBP-rXKW5=tNJLaSd0q8+Vm=CzNfsP2nR0sJg@mail.gmail.com","threadId":"61016","inReplyTo":"95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-02-28T11:32:35Z","receivedAt":"2024-02-28T11:32:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Under normal circumstances, it shouldn't ever happen that a repository\n> has no HEAD reference. In fact, git-update-ref(1) would fail any request\n> to delete the HEAD reference, and a newly initialized repository always\n> pre-creates it, too.\n>\n> But in the next commit, we are going to change git-clone(1) to partially\n> initialize the refdb just up to the point where remote helpers can find\n> the repository. With that change, we are going to run into a situation\n> where repositories have no refs at all.\n>\n> Now there is a very particular edge case in this situation: when\n> preparing an empty ref transacton, we end up returning whatever value\n> `read_ref_without_reload()` returned to the caller. Under normal\n> conditions this would be fine: \"HEAD\" should usually exist, and thus the\n> function would return `0`. But if \"HEAD\" doesn't exist, the function\n> returns a positive value which we end up returning to the caller.\n>\n> Fix this bug by resetting the return code to `0` and add a test.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/reftable-backend.c | 1 +\n> t/t0610-reftable-basics.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+)\n>\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index a14f2ad7f4..45568818f0 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t\t &head_referent, &head_type);\n> \tif (ret < 0)\n> \t\tgoto done;\n> +\tret = 0;\n>\n\nSo this is not really a problem in this function, it's more of that\n`refs.c:ref_transaction_prepare` checks if `ret` is non-zero.\n\nNit: would be nice to have a comment about why overriding this value is\nok.\n\n> \tfor (i = 0; i < transaction->nr; i++) {\n> \t\tstruct ref_update *u = transaction->updates[i];\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 6a131e40b8..c5f4d23433 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -328,6 +328,19 @@ test_expect_success 'ref transaction: writes are synced' '\n> \tEOF\n> '\n>\n> +test_expect_success 'ref transaction: empty transaction in empty repo' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\ttest_commit -C repo --no-tag A &&\n> +\tCOMMIT=$(git -C repo rev-parse HEAD) &&\n\nwhy do we do this?\n"},{"id":"489719","messageId":"xcbwno3x7tzcg77rtfkaqtqcgll32vhx5mlhcvlclew6lkkofu@h5jzvft55uvv","threadId":"61016","inReplyTo":"95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-03-01T01:20:42Z","receivedAt":"2024-03-01T01:21:24Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/02/27 03:27PM, Patrick Steinhardt wrote:\n> Under normal circumstances, it shouldn't ever happen that a repository\n> has no HEAD reference. In fact, git-update-ref(1) would fail any request\n> to delete the HEAD reference, and a newly initialized repository always\n> pre-creates it, too.\n> \n> But in the next commit, we are going to change git-clone(1) to partially\n> initialize the refdb just up to the point where remote helpers can find\n> the repository. With that change, we are going to run into a situation\n> where repositories have no refs at all.\n> \n> Now there is a very particular edge case in this situation: when\n> preparing an empty ref transacton, we end up returning whatever value\n> `read_ref_without_reload()` returned to the caller. Under normal\n> conditions this would be fine: \"HEAD\" should usually exist, and thus the\n> function would return `0`. But if \"HEAD\" doesn't exist, the function\n> returns a positive value which we end up returning to the caller.\n\nIn what context are reference transactions being created before the\nrefdb is fully initialized? Is this in regards to repositories\ninitialized with the reftables backend and used during execution of a\nremote-helper? I was originally under the impression that a partially\ninitalized refdb would appear as the reffile backend before being fully\ninitialized.\n> \n> Fix this bug by resetting the return code to `0` and add a test.\n> \n> Signed-off-by: Patrick Steinhardt \n\n-Justin\n"},{"id":"489834","messageId":"ZeVtohtcxCCqUSlX@tanuki","threadId":"61016","inReplyTo":"xcbwno3x7tzcg77rtfkaqtqcgll32vhx5mlhcvlclew6lkkofu@h5jzvft55uvv","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T06:44:02Z","receivedAt":"2024-03-04T06:44:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 29, 2024 at 07:20:42PM -0600, Justin Tobler wrote:\n> On 24/02/27 03:27PM, Patrick Steinhardt wrote:\n> > Under normal circumstances, it shouldn't ever happen that a repository\n> > has no HEAD reference. In fact, git-update-ref(1) would fail any request\n> > to delete the HEAD reference, and a newly initialized repository always\n> > pre-creates it, too.\n> > \n> > But in the next commit, we are going to change git-clone(1) to partially\n> > initialize the refdb just up to the point where remote helpers can find\n> > the repository. With that change, we are going to run into a situation\n> > where repositories have no refs at all.\n> > \n> > Now there is a very particular edge case in this situation: when\n> > preparing an empty ref transacton, we end up returning whatever value\n> > `read_ref_without_reload()` returned to the caller. Under normal\n> > conditions this would be fine: \"HEAD\" should usually exist, and thus the\n> > function would return `0`. But if \"HEAD\" doesn't exist, the function\n> > returns a positive value which we end up returning to the caller.\n> \n> In what context are reference transactions being created before the\n> refdb is fully initialized? Is this in regards to repositories\n> initialized with the reftables backend and used during execution of a\n> remote-helper? I was originally under the impression that a partially\n> initalized refdb would appear as the reffile backend before being fully\n> initialized.\n\nThe only case where this can happen is during git-clone(1).\n\nPatrick\n\n> > Fix this bug by resetting the return code to `0` and add a test.\n> > \n> > Signed-off-by: Patrick Steinhardt \n> \n> -Justin\n"},{"id":"489835","messageId":"ZeVtuqEAelfiA2J9@tanuki","threadId":"61016","inReplyTo":"CAOLa=ZSycN0iYbBP-rXKW5=tNJLaSd0q8+Vm=CzNfsP2nR0sJg@mail.gmail.com","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T06:44:10Z","receivedAt":"2024-03-04T06:44:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 28, 2024 at 03:32:35AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Under normal circumstances, it shouldn't ever happen that a repository\n> > has no HEAD reference. In fact, git-update-ref(1) would fail any request\n> > to delete the HEAD reference, and a newly initialized repository always\n> > pre-creates it, too.\n> >\n> > But in the next commit, we are going to change git-clone(1) to partially\n> > initialize the refdb just up to the point where remote helpers can find\n> > the repository. With that change, we are going to run into a situation\n> > where repositories have no refs at all.\n> >\n> > Now there is a very particular edge case in this situation: when\n> > preparing an empty ref transacton, we end up returning whatever value\n> > `read_ref_without_reload()` returned to the caller. Under normal\n> > conditions this would be fine: \"HEAD\" should usually exist, and thus the\n> > function would return `0`. But if \"HEAD\" doesn't exist, the function\n> > returns a positive value which we end up returning to the caller.\n> >\n> > Fix this bug by resetting the return code to `0` and add a test.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > refs/reftable-backend.c | 1 +\n> > t/t0610-reftable-basics.sh | 13 +++++++++++++\n> > 2 files changed, 14 insertions(+)\n> >\n> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> > index a14f2ad7f4..45568818f0 100644\n> > --- a/refs/reftable-backend.c\n> > +++ b/refs/reftable-backend.c\n> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n> > \t\t\t\t &head_referent, &head_type);\n> > \tif (ret < 0)\n> > \t\tgoto done;\n> > +\tret = 0;\n> >\n> \n> So this is not really a problem in this function, it's more of that\n> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.\n\nWell, yes. I'd claim that it is a problem in this function because it\nreturns positive even though the transaction was prepared successfully.\n\n> Nit: would be nice to have a comment about why overriding this value is\n> ok.\n\nTrue.\n\n> > \tfor (i = 0; i < transaction->nr; i++) {\n> > \t\tstruct ref_update *u = transaction->updates[i];\n> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> > index 6a131e40b8..c5f4d23433 100755\n> > --- a/t/t0610-reftable-basics.sh\n> > +++ b/t/t0610-reftable-basics.sh\n> > @@ -328,6 +328,19 @@ test_expect_success 'ref transaction: writes are synced' '\n> > \tEOF\n> > '\n> >\n> > +test_expect_success 'ref transaction: empty transaction in empty repo' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\ttest_commit -C repo --no-tag A &&\n> > +\tCOMMIT=$(git -C repo rev-parse HEAD) &&\n> \n> why do we do this?\n\nOh, true, this isn't actually needed.\n\nPatrick\n"},{"id":"489836","messageId":"ZeVtwjr9ovUa6-8z@tanuki","threadId":"61016","inReplyTo":"xmqqh6htpp6k.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/clone: allow remote helpers to detect repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T06:44:18Z","receivedAt":"2024-03-04T06:44:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 27, 2024 at 01:33:55PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Patrick Steinhardt writes:\n> >\n> >> Hi,\n> >>\n> >> this patch series addresses a regression reported by Mike in Git v2.44\n> >> where remote helpers cannot access the Git repository anymore when\n> >> running git-clone(1).\n> >> ...\n> >> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n> >> refs/reftable-backend.c | 1 +\n> >\n> > Sorry, but this confuses me. Was a regression really in v2.44.0,\n> > where refs/reftable-backend.c did not even exist? If so why does a\n> > fix for it need to touch that file?\n> >\n> > Thanks.\n> \n> I guess [2/2] alone is the fix to be applied directly on top of v2.44.0\n> and eventually be merged to 'maint' to become v2.44.1 release, while\n> [1/2] is necessary to adjust reftable backend when such a fix is\n> merged to more recent codebase that already has the reftable\n> backend?\n\nYes, exactly! Sorry, should've explained this more thoroughly.\n\nPatrick\n"},{"id":"489837","messageId":"ZeVt0CAfpYFZqT2i@tanuki","threadId":"61016","inReplyTo":"xmqqle75ppa3.fsf@gitster.g","subject":"Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-04T06:44:32Z","receivedAt":"2024-03-04T06:44:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 27, 2024 at 01:31:48PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > Instead, fix this issue by partially initializing the refdb up to the\n> > point where it becomes discoverable by Git commands.\n> \n> As you mentioned earlier, this indeed is ugly, but I agree that\n> there is no other reasonable way out. I am also unsure if this is a\n> viable workaround in the longer term, but it should do for now.\n> \n> > +\t/*\n> > +\t * We have a chicken-and-egg situation between initializing the refdb\n> > +\t * and spawning transport helpers:\n> > +\t *\n> > +\t * - Initializing the refdb requires us to know about the object\n> > +\t * format. We thus have to spawn the transport helper to learn\n> > +\t * about it.\n> > +\t * - The transport helper may want to access the Git repository. But\n> > +\t * because the refdb has not been initialized, we don't have \"HEAD\"\n> > +\t * or \"refs/\". Thus, the helper cannot find the Git repository.\n> > +\t *\n> > +\t * Ideally, we would have structured the helper protocol such that it's\n> > +\t * mandatory for the helper to first announce its capabilities without\n> > +\t * yet assuming a fully initialized repository. Like that, we could\n> > +\t * have added a \"lazy-refdb-init\" capability that announces whether the\n> > +\t * helper is ready to handle not-yet-initialized refdbs. If any helper\n> > +\t * didn't support them, we would have fully initialized the refdb with\n> > +\t * the SHA1 object format, but later on bailed out if we found out that\n> > +\t * the remote repository used a different object format.\n> > +\t * But we didn't, and thus we use the following workaround to partially\n> > +\t * initialize the repository's refdb such that it can be discovered by\n> > +\t * Git commands. To do so, we:\n> > +\t *\n> > +\t * - Create an invalid HEAD ref pointing at \"refs/heads/.invalid\".\n> > +\t *\n> > +\t * - Create the \"refs/\" directory.\n> > +\t *\n> > +\t * - Set up the ref storage format and repository version as\n> > +\t * required.\n> \n> \"as required\" by whom and in what way? \n> \n> \"The code to recognize a repository requires them to be set already,\n> but they do not have to be the real value---we just assign random\n> valid values for now, let remote helper do its work and then set the\n> real values after they are done\" would be a plausible interpretation\n> of the above. Is that what is going on?\n\nPartially. While we cannot yet determine the object format, we do know\nthe ref storage format as it is specified by the caller when invoking\ngit-clone(1) with the `--ref-format=` switch, not by the remote repo.\nIn that case, we'd have to set up the ref format accordingly and also\nset the repository version to \"1\".\n\nPatrick\n\n> > +\t * This is sufficient for Git commands to discover the Git directory.\n> > +\t */\n> > +\tinitialize_repository_version(GIT_HASH_UNKNOWN,\n> > +\t\t\t\t the_repository->ref_storage_format, 1);\n> \n> OK, so we start with \"we do not know it yet\" here.\n> \n> > +\tstrbuf_addf(&buf, \"%s/HEAD\", git_dir);\n> > +\twrite_file(buf.buf, \"ref: refs/heads/.invalid\");\n> > +\n> > +\tstrbuf_reset(&buf);\n> > +\tstrbuf_addf(&buf, \"%s/refs\", git_dir);\n> > +\tsafe_create_dir(buf.buf, 1);\n> > +\n> > \t/*\n> > \t * additional config can be injected with -c, make sure it's included\n> > \t * after init_db, which clears the entire config environment.\n> > @@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> > \tfree(remote_name);\n> > \tstrbuf_release(&reflog_msg);\n> > \tstrbuf_release(&branch_top);\n> > +\tstrbuf_release(&buf);\n> > \tstrbuf_release(&key);\n> > \tfree_refs(mapped_refs);\n> > \tfree_refs(remote_head_points_at);\n> > diff --git a/setup.c b/setup.c\n> > index b69b1cbc2a..e3b76e84b5 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,\n> > \tchar repo_version_string[10];\n> > \tint repo_version = GIT_REPO_VERSION;\n> > \n> > +\t/*\n> > +\t * Note that we initialize the repository version to 1 when the ref\n> > +\t * storage format is unknown. This is on purpose so that we can add the\n> > +\t * correct object format to the config during git-clone(1). The format\n> > +\t * version will get adjusted by git-clone(1) once it has learned about\n> > +\t * the remote repository's format.\n> > +\t */\n> > \tif (hash_algo != GIT_HASH_SHA1 ||\n> > \t ref_storage_format != REF_STORAGE_FORMAT_FILES)\n> > \t\trepo_version = GIT_REPO_VERSION_READ;\n> > @@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,\n> > \t\t \"%d\", repo_version);\n> > \tgit_config_set(\"core.repositoryformatversion\", repo_version_string);\n> > \n> > -\tif (hash_algo != GIT_HASH_SHA1)\n> > +\tif (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)\n> > \t\tgit_config_set(\"extensions.objectformat\",\n> > \t\t\t hash_algos[hash_algo].name);\n> > \telse if (reinit)\n> \n> Here we refrain from writing extensions.objectformat in the\n> repository when we do not know what hash function is being used.\n> Without this change, we would instead remove extensions.objectformat,\n> which does not sound too bad, as \"reinit\" is passed by the new call\n> we saw above and this \"else if (reinit)\" will do exactly that anyway.\n> \n> In any case, after we finish talking with the other side over the\n> transport, we make another call to initialize_repository_version()\n> with the real objectformat and everything will be fine.\n> \n> The ealier description of the implementation idea made it sound\n> really yucky, but I do not think the solution presented here is too\n> bad.\n> \n> > diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> > index 1544d6dc6b..bcfb358c51 100755\n> > --- a/t/t5801/git-remote-testgit\n> > +++ b/t/t5801/git-remote-testgit\n> > @@ -12,6 +12,11 @@ url=$2\n> > \n> > dir=\"$GIT_DIR/testgit/$alias\"\n> > \n> > +if ! git rev-parse --is-inside-git-dir\n> > +then\n> > +\texit 1\n> > +fi\n> > +\n> > h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n> > t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n"},{"id":"489898","messageId":"xmqqbk7urmxu.fsf@gitster.g","threadId":"61016","inReplyTo":"ZeVt0CAfpYFZqT2i@tanuki","subject":"Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T16:17:33Z","receivedAt":"2024-03-04T16:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> > +\t * Git commands. To do so, we:\n>> > +\t *\n>> > +\t * - Create an invalid HEAD ref pointing at \"refs/heads/.invalid\".\n>> > +\t *\n>> > +\t * - Create the \"refs/\" directory.\n>> > +\t *\n>> > +\t * - Set up the ref storage format and repository version as\n>> > +\t * required.\n>> \n>> \"as required\" by whom and in what way? \n>> \n>> \"The code to recognize a repository requires them to be set already,\n>> but they do not have to be the real value---we just assign random\n>> valid values for now, let remote helper do its work and then set the\n>> real values after they are done\" would be a plausible interpretation\n>> of the above. Is that what is going on?\n>\n> Partially. While we cannot yet determine the object format, we do know\n> the ref storage format as it is specified by the caller when invoking\n> git-clone(1) with the `--ref-format=` switch, not by the remote repo.\n> In that case, we'd have to set up the ref format accordingly and also\n> set the repository version to \"1\".\n\nAh, of course. Unlike the object format, we do not say \"the other\nend uses reftable, so we should do the same\" and we do not have to,\nthanks to some artificial limitations added to the reftable backend\nnot to exceed the expressiveness of the files backend.\n\nThanks.\n"},{"id":"489899","messageId":"xmqq5xy2rmfy.fsf@gitster.g","threadId":"61016","inReplyTo":"ZeVtuqEAelfiA2J9@tanuki","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-04T16:28:17Z","receivedAt":"2024-03-04T16:28:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> > Now there is a very particular edge case in this situation: when\n>> > preparing an empty ref transacton, we end up returning whatever value\n>> > `read_ref_without_reload()` returned to the caller. Under normal\n>> > conditions this would be fine: \"HEAD\" should usually exist, and thus the\n>> > function would return `0`. But if \"HEAD\" doesn't exist, the function\n>> > returns a positive value which we end up returning to the caller.\n>> >\n>> > Fix this bug by resetting the return code to `0` and add a test.\n\nSo this _will_ surface as a bug when the other change in the series\nis applied, but it nevertheless is worth fixing independently of the\nother one, because ...\n\n>> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n>> > \t\t\t\t &head_referent, &head_type);\n>> > \tif (ret < 0)\n>> > \t\tgoto done;\n>> > +\tret = 0;\n\n... after \"if the refs->err records an error already, skip\neverything we do and return to the caller\", we should take the\nownership of what we return (which will be in \"ret\") from now on.\n\nSo the current code uses \"ret\" as an uninitialized variable, even\nnot technically so because it is \"initialized\" to \"refs->err\"\nupfront, and this is like a fix of uninitialized variable use.\n\n>> So this is not really a problem in this function, it's more of that\n>> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.\n>\n> Well, yes. I'd claim that it is a problem in this function because it\n> returns positive even though the transaction was prepared successfully.\n>\n>> Nit: would be nice to have a comment about why overriding this value is\n>> ok.\n>\n> True.\n\nYup. It seems we will see a v2 for updating the test code as well,\nso I'll assume that you'd explain this as an independent fix (as\nwell as a required preliminary fix for the other one).\n\nThanks, both.\n"},{"id":"489951","messageId":"ZecFXXqUdGEQ3YhC@tanuki","threadId":"61016","inReplyTo":"xmqq5xy2rmfy.fsf@gitster.g","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T11:43:25Z","receivedAt":"2024-03-05T11:43:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 04, 2024 at 08:28:17AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> > Now there is a very particular edge case in this situation: when\n> >> > preparing an empty ref transacton, we end up returning whatever value\n> >> > `read_ref_without_reload()` returned to the caller. Under normal\n> >> > conditions this would be fine: \"HEAD\" should usually exist, and thus the\n> >> > function would return `0`. But if \"HEAD\" doesn't exist, the function\n> >> > returns a positive value which we end up returning to the caller.\n> >> >\n> >> > Fix this bug by resetting the return code to `0` and add a test.\n> \n> So this _will_ surface as a bug when the other change in the series\n> is applied, but it nevertheless is worth fixing independently of the\n> other one, because ...\n> \n> >> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n> >> > \t\t\t\t &head_referent, &head_type);\n> >> > \tif (ret < 0)\n> >> > \t\tgoto done;\n> >> > +\tret = 0;\n> \n> ... after \"if the refs->err records an error already, skip\n> everything we do and return to the caller\", we should take the\n> ownership of what we return (which will be in \"ret\") from now on.\n> \n> So the current code uses \"ret\" as an uninitialized variable, even\n> not technically so because it is \"initialized\" to \"refs->err\"\n> upfront, and this is like a fix of uninitialized variable use.\n\nThe problem is a bit different. We call `read_ref_without_reload()` to\nlook up the \"HEAD\" ref, which will return a positive value in case the\nref wasn't found. This is customary in the reftable library: positive\nerror values indicate that an iter is over, and thus by extension that a\nvalue was not found. It's fine though if the ref doesn't exist, and we\nhandle that case gracefully.\n\nThe only exception is when the transaction is also empty. In that case,\nwe skip the loop and thus end up not assigning to `ret` anymore. Thus,\nthe positive error code we still have in `ret` from the failed \"HEAD\"\nlookup gets returned to the caller, which is wrong.\n\nSo it's not uninitialized, it rather is stale.\n\nBut yes, the bug _can_ be hit independently of the second patch in this\nseries. It's just really unlikely as a repo without \"HEAD\" is considered\nto be broken anyway.\n\n> >> So this is not really a problem in this function, it's more of that\n> >> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.\n> >\n> > Well, yes. I'd claim that it is a problem in this function because it\n> > returns positive even though the transaction was prepared successfully.\n> >\n> >> Nit: would be nice to have a comment about why overriding this value is\n> >> ok.\n> >\n> > True.\n> \n> Yup. It seems we will see a v2 for updating the test code as well,\n> so I'll assume that you'd explain this as an independent fix (as\n> well as a required preliminary fix for the other one).\n\nI see that the patch series has been merged to \"next\" a few days ago\nthough, and is slated to be merged to \"master\". That's why I refrained\nfrom sending a v2.\n\nI can send a follow-up patch to remove the useless variable assignment\nin the test, but other than that I don't think anything needs to change\nhere. Or did I miss anything else?\n\nPatrick\n"},{"id":"489963","messageId":"xmqqmsrc655o.fsf@gitster.g","threadId":"61016","inReplyTo":"ZecFXXqUdGEQ3YhC@tanuki","subject":"Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-05T15:59:31Z","receivedAt":"2024-03-05T15:59:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> So it's not uninitialized, it rather is stale.\n\nWhat I wanted to say with \"technically it is not\" was that these two\nare moral equivalents in the code around there after the early return\nhappened ;-).\n\n> I can send a follow-up patch to remove the useless variable assignment\n> in the test, but other than that I don't think anything needs to change\n> here.\n\nOK, thanks.\n\n"},{"id":"490067","messageId":"755ea17d3d88462e8c0a9a8338a0c82ad6506338.1709723814.git.ps@pks.im","threadId":"61016","inReplyTo":"ZecFXXqUdGEQ3YhC@tanuki","subject":"[PATCH] t0610: remove unused variable assignment","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-06T11:17:27Z","receivedAt":"2024-03-06T11:17:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In b0f6b6b523 (refs/reftable: don't fail empty transactions in repo\nwithout HEAD, 2024-02-27), we have added a new test to t0610. This test\ncontains a useless assignment to a variable that is never actually used.\nRemove it.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t0610-reftable-basics.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex c5f4d23433..686781192e 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -332,7 +332,6 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \ttest_commit -C repo --no-tag A &&\n-\tCOMMIT=$(git -C repo rev-parse HEAD) &&\n \tgit -C repo update-ref -d refs/heads/main &&\n \ttest-tool -C repo ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n \tgit -C repo update-ref --stdin <<-EOF\n-- \n2.44.0\n\n"},{"id":"493954","messageId":"20240503020432.2fxwuhjsvumy7i7z@glandium.org","threadId":"61016","inReplyTo":"9d888adf92e9a8af7c18847219f97d3e595e3e36.1709041721.git.ps@pks.im","subject":"Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2024-05-03T02:04:32Z","receivedAt":"2024-05-03T02:23:22Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hey, it's me again, coming back way too late to figure out that this fix\nhad an adverse side effect in a corner case.\n\nHere's a simplified reproducer:\n```\n$ cat > git-remote-foo <\nhint:\nhint: Names commonly chosen instead of 'master' are 'main', 'trunk' and\nhint: 'development'. The just-created branch can be renamed via this command:\nhint:\nhint: \tgit branch -m \n```\n\nThat hint is the adverse side effect. It shouldn't be shown.\n\nIt's happening because `builtin/clone.c` does this:\n\t`create_reference_database(the_repository->ref_storage_format, NULL, 1);`\n\n`create_reference` calls `is_reinit`, which returns true because there is\nHEAD, and proceeds to skip the block behind `if (!reinit)`.\n\nBefore the change, that code block that is now skipped would have called\n`git_default_branch_name(quiet)` (where `quiet` is 1, per the above call\nto `create_reference_database`). In turn\n`git_default_branch_name(quiet)` would have a) skipped the initial\nbranch hint (because quiet), and b) cached the branch name for subsequent\n`git_default_branch_name` calls.\n\nThen later in `builtin/clone.c`, we end up in this code:\n\t`branch = git_default_branch_name(0);`\n\nBefore the change, this would get the cache value from the previous\ncall, but now it has to compute it, and because 0 is passed, it's not\nquiet about the initial branch hint.\n\nThe non-remote-helper case goes through with this without a problem\nbecause it ends up in the transport_ls_refs_options.unborn_head_target\ncase and never calls `git_default_branch_name(0)`.\n\nMike\n\nOn Tue, Feb 27, 2024 at 03:27:44PM +0100, Patrick Steinhardt wrote:\n> In 18c9cb7524 (builtin/clone: create the refdb with the correct object\n> format, 2023-12-12), we have changed git-clone(1) so that it delays\n> creation of the refdb until after it has learned about the remote's\n> object format. This change was required for the reftable backend, which\n> encodes the object format into the tables. So if we pre-initialized the\n> refdb with the default object format, but the remote uses a different\n> object format than that, then the resulting tables would have encoded\n> the wrong object format.\n> \n> This change unfortunately breaks remote helpers which try to access the\n> repository that is about to be created. Because the refdb has not yet\n> been initialized at the point where we spawn the remote helper, we also\n> don't yet have \"HEAD\" or \"refs/\". Consequently, any Git commands ran by\n> the remote helper which try to access the repository would fail because\n> it cannot be discovered.\n> \n> This is essentially a chicken-and-egg problem: we cannot initialize the\n> refdb because we don't know about the object format. But we cannot learn\n> about the object format because the remote helper may be unable to\n> access the partially-initialized repository.\n> \n> Ideally, we would address this issue via capabilities. But the remote\n> helper protocol is not structured in a way that guarantees that the\n> capability announcement happens before the remote helper tries to access\n> the repository.\n> \n> Instead, fix this issue by partially initializing the refdb up to the\n> point where it becomes discoverable by Git commands.\n> \n> Reported-by: Mike Hommey \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/clone.c | 46 ++++++++++++++++++++++++++++++++++++++\n> setup.c | 9 +++++++-\n> t/t5801/git-remote-testgit | 5 +++++\n> 3 files changed, 59 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index bad1b70ce8..5d7f112125 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -926,6 +926,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tstruct ref *mapped_refs = NULL;\n> \tconst struct ref *ref;\n> \tstruct strbuf key = STRBUF_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n> \tstruct transport *transport = NULL;\n> \tconst char *src_ref_prefix = \"refs/heads/\";\n> @@ -1125,6 +1126,50 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\tgit_dir = real_git_dir;\n> \t}\n> \n> +\t/*\n> +\t * We have a chicken-and-egg situation between initializing the refdb\n> +\t * and spawning transport helpers:\n> +\t *\n> +\t * - Initializing the refdb requires us to know about the object\n> +\t * format. We thus have to spawn the transport helper to learn\n> +\t * about it.\n> +\t *\n> +\t * - The transport helper may want to access the Git repository. But\n> +\t * because the refdb has not been initialized, we don't have \"HEAD\"\n> +\t * or \"refs/\". Thus, the helper cannot find the Git repository.\n> +\t *\n> +\t * Ideally, we would have structured the helper protocol such that it's\n> +\t * mandatory for the helper to first announce its capabilities without\n> +\t * yet assuming a fully initialized repository. Like that, we could\n> +\t * have added a \"lazy-refdb-init\" capability that announces whether the\n> +\t * helper is ready to handle not-yet-initialized refdbs. If any helper\n> +\t * didn't support them, we would have fully initialized the refdb with\n> +\t * the SHA1 object format, but later on bailed out if we found out that\n> +\t * the remote repository used a different object format.\n> +\t *\n> +\t * But we didn't, and thus we use the following workaround to partially\n> +\t * initialize the repository's refdb such that it can be discovered by\n> +\t * Git commands. To do so, we:\n> +\t *\n> +\t * - Create an invalid HEAD ref pointing at \"refs/heads/.invalid\".\n> +\t *\n> +\t * - Create the \"refs/\" directory.\n> +\t *\n> +\t * - Set up the ref storage format and repository version as\n> +\t * required.\n> +\t *\n> +\t * This is sufficient for Git commands to discover the Git directory.\n> +\t */\n> +\tinitialize_repository_version(GIT_HASH_UNKNOWN,\n> +\t\t\t\t the_repository->ref_storage_format, 1);\n> +\n> +\tstrbuf_addf(&buf, \"%s/HEAD\", git_dir);\n> +\twrite_file(buf.buf, \"ref: refs/heads/.invalid\");\n> +\n> +\tstrbuf_reset(&buf);\n> +\tstrbuf_addf(&buf, \"%s/refs\", git_dir);\n> +\tsafe_create_dir(buf.buf, 1);\n> +\n> \t/*\n> \t * additional config can be injected with -c, make sure it's included\n> \t * after init_db, which clears the entire config environment.\n> @@ -1453,6 +1498,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tfree(remote_name);\n> \tstrbuf_release(&reflog_msg);\n> \tstrbuf_release(&branch_top);\n> +\tstrbuf_release(&buf);\n> \tstrbuf_release(&key);\n> \tfree_refs(mapped_refs);\n> \tfree_refs(remote_head_points_at);\n> diff --git a/setup.c b/setup.c\n> index b69b1cbc2a..e3b76e84b5 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1889,6 +1889,13 @@ void initialize_repository_version(int hash_algo,\n> \tchar repo_version_string[10];\n> \tint repo_version = GIT_REPO_VERSION;\n> \n> +\t/*\n> +\t * Note that we initialize the repository version to 1 when the ref\n> +\t * storage format is unknown. This is on purpose so that we can add the\n> +\t * correct object format to the config during git-clone(1). The format\n> +\t * version will get adjusted by git-clone(1) once it has learned about\n> +\t * the remote repository's format.\n> +\t */\n> \tif (hash_algo != GIT_HASH_SHA1 ||\n> \t ref_storage_format != REF_STORAGE_FORMAT_FILES)\n> \t\trepo_version = GIT_REPO_VERSION_READ;\n> @@ -1898,7 +1905,7 @@ void initialize_repository_version(int hash_algo,\n> \t\t \"%d\", repo_version);\n> \tgit_config_set(\"core.repositoryformatversion\", repo_version_string);\n> \n> -\tif (hash_algo != GIT_HASH_SHA1)\n> +\tif (hash_algo != GIT_HASH_SHA1 && hash_algo != GIT_HASH_UNKNOWN)\n> \t\tgit_config_set(\"extensions.objectformat\",\n> \t\t\t hash_algos[hash_algo].name);\n> \telse if (reinit)\n> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> index 1544d6dc6b..bcfb358c51 100755\n> --- a/t/t5801/git-remote-testgit\n> +++ b/t/t5801/git-remote-testgit\n> @@ -12,6 +12,11 @@ url=$2\n> \n> dir=\"$GIT_DIR/testgit/$alias\"\n> \n> +if ! git rev-parse --is-inside-git-dir\n> +then\n> +\texit 1\n> +fi\n> +\n> h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n> t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n> \n> -- \n> 2.44.0\n> \n\n\n"}]}