{"thread":{"id":"57755","subject":"[PATCH] clone: ignore invalid local refs in remote","startedAt":"2022-04-18T17:12:54Z","lastAt":"2022-04-25T13:47:48Z","messageCount":6,"participants":["Derrick Stolee via GitGitGadget","Ævar Arnfjörð Bjarmason","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453853","messageId":"pull.1214.git.1650301959803.gitgitgadget@gmail.com","threadId":"57755","inReplyTo":null,"subject":"[PATCH] clone: ignore invalid local refs in remote","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-18T17:12:39Z","receivedAt":"2022-04-18T17:12:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen cloning directly from a local repository, we load a list of refs\nbased on scanning the $GIT_DIR/refs/ directory of the \"server\"\nrepository. If files exist in that directory that do not parse as\nhexadecimal hashes, then the ref array used by write_remote_refs()\nends up with some entries with null OIDs. This causes us to hit a BUG()\nstatement in ref_transaction_create():\n\n  BUG: create called without valid new_oid\n\nThis BUG() call used to be a die() until 033abf97f (Replace all\ndie(\"BUG: ...\") calls by BUG() ones, 2018-05-02). Before that, the die()\nwas added by f04c5b552 (ref_transaction_create(): check that new_sha1 is\nvalid, 2015-02-17).\n\nThe original report for this bug [1] mentioned that this problem did not\nexist in Git 2.27.0. The failure bisects unsurprisingly to 968f12fda\n(refs: turn on GIT_REF_PARANOIA by default, 2021-09-24). When\nGIT_REF_PARANOIA is enabled, this case always fails as far back as I am\nable to successfully compile and test the Git codebase.\n\n[1] https://github.com/git-for-windows/git/issues/3781\n\nThere are two approaches to consider here. One would be to remove this\nBUG() statement in favor of returning with an error. There are only two\ncallers to ref_transaction_create(), so this would have a limited\nimpact.\n\nThe other approach would be to add special casing in 'git clone' to\navoid this faulty input to the method.\n\nWhile I originally started with changing 'git clone', I decided that\nmodifying ref_transaction_create() was a more complete solution. This\nprevents failing with a BUG() statement when we already have a good way\nto report an error (including a reason for that error) within the\nmethod. Both callers properly check the return value and die() with the\nerror message, so this is an appropriate direction.\n\nThe added test helps check against a regression, but does check that our\nintended error message is handled correctly.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    clone: ignore invalid local refs in remote\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1214%2Fderrickstolee%2Frefs-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1214/derrickstolee/refs-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1214\n\n refs.c                 | 6 ++++--\n t/t5605-clone-local.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1a964505f92..f300f83e4d4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1111,8 +1111,10 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n-\tif (!new_oid || is_null_oid(new_oid))\n-\t\tBUG(\"create called without valid new_oid\");\n+\tif (!new_oid || is_null_oid(new_oid)) {\n+\t\tstrbuf_addf(err, \"'%s' has a null OID\", refname);\n+\t\treturn 1;\n+\t}\n \treturn ref_transaction_update(transaction, refname, new_oid,\n \t\t\t\t      null_oid(), flags, msg, err);\n }\ndiff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\nindex 7d63365f93a..21ab6192839 100755\n--- a/t/t5605-clone-local.sh\n+++ b/t/t5605-clone-local.sh\n@@ -141,4 +141,13 @@ test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n \ttest_must_fail git clone --bare -u false a should_not_work.git\n '\n \n+test_expect_success 'local clone from repo with corrupt refs fails gracefully' '\n+\tgit init corrupt &&\n+\ttest_commit -C corrupt one &&\n+\techo a >corrupt/.git/refs/heads/topic &&\n+\n+\ttest_must_fail git clone corrupt working 2>err &&\n+\tgrep \"has a null OID\" err\n+'\n+\n test_done\n\nbase-commit: 11cfe552610386954886543f5de87dcc49ad5735\n-- \ngitgitgadget\n"},{"id":"453894","messageId":"220418.86zgki8ffb.gmgdl@evledraar.gmail.com","threadId":"57755","inReplyTo":"pull.1214.git.1650301959803.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: ignore invalid local refs in remote","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-18T17:30:35Z","receivedAt":"2022-04-18T17:33:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 18 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> When cloning directly from a local repository, we load a list of refs\n> based on scanning the $GIT_DIR/refs/ directory of the \"server\"\n> repository. If files exist in that directory that do not parse as\n> hexadecimal hashes, then the ref array used by write_remote_refs()\n> ends up with some entries with null OIDs. This causes us to hit a BUG()\n> statement in ref_transaction_create():\n>\n>   BUG: create called without valid new_oid\n>\n> This BUG() call used to be a die() until 033abf97f (Replace all\n> die(\"BUG: ...\") calls by BUG() ones, 2018-05-02). Before that, the die()\n> was added by f04c5b552 (ref_transaction_create(): check that new_sha1 is\n> valid, 2015-02-17).\n>\n> The original report for this bug [1] mentioned that this problem did not\n> exist in Git 2.27.0. The failure bisects unsurprisingly to 968f12fda\n> (refs: turn on GIT_REF_PARANOIA by default, 2021-09-24). When\n> GIT_REF_PARANOIA is enabled, this case always fails as far back as I am\n> able to successfully compile and test the Git codebase.\n>\n> [1] https://github.com/git-for-windows/git/issues/3781\n>\n> There are two approaches to consider here. One would be to remove this\n> BUG() statement in favor of returning with an error. There are only two\n> callers to ref_transaction_create(), so this would have a limited\n> impact.\n>\n> The other approach would be to add special casing in 'git clone' to\n> avoid this faulty input to the method.\n>\n> While I originally started with changing 'git clone', I decided that\n> modifying ref_transaction_create() was a more complete solution. This\n> prevents failing with a BUG() statement when we already have a good way\n> to report an error (including a reason for that error) within the\n> method. Both callers properly check the return value and die() with the\n> error message, so this is an appropriate direction.\n>\n> The added test helps check against a regression, but does check that our\n> intended error message is handled correctly.\n>\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>     clone: ignore invalid local refs in remote\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1214%2Fderrickstolee%2Frefs-bug-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1214/derrickstolee/refs-bug-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1214\n>\n>  refs.c                 | 6 ++++--\n>  t/t5605-clone-local.sh | 9 +++++++++\n>  2 files changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 1a964505f92..f300f83e4d4 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1111,8 +1111,10 @@ int ref_transaction_create(struct ref_transaction *transaction,\n>  \t\t\t   unsigned int flags, const char *msg,\n>  \t\t\t   struct strbuf *err)\n>  {\n> -\tif (!new_oid || is_null_oid(new_oid))\n> -\t\tBUG(\"create called without valid new_oid\");\n> +\tif (!new_oid || is_null_oid(new_oid)) {\n> +\t\tstrbuf_addf(err, \"'%s' has a null OID\", refname);\n> +\t\treturn 1;\n> +\t}\n>  \treturn ref_transaction_update(transaction, refname, new_oid,\n>  \t\t\t\t      null_oid(), flags, msg, err);\n>  }\n> diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\n> index 7d63365f93a..21ab6192839 100755\n> --- a/t/t5605-clone-local.sh\n> +++ b/t/t5605-clone-local.sh\n> @@ -141,4 +141,13 @@ test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n>  \ttest_must_fail git clone --bare -u false a should_not_work.git\n>  '\n>  \n> +test_expect_success 'local clone from repo with corrupt refs fails gracefully' '\n> +\tgit init corrupt &&\n> +\ttest_commit -C corrupt one &&\n> +\techo a >corrupt/.git/refs/heads/topic &&\n> +\n> +\ttest_must_fail git clone corrupt working 2>err &&\n> +\tgrep \"has a null OID\" err\n> +'\n> +\n>  test_done\n>\n> base-commit: 11cfe552610386954886543f5de87dcc49ad5735\n\nIf I change this test to clone from \"$PWD/corrupt\" instead we get as far\nas dying in \"not our ref\" in upload-pack\", after the \"client\" said \"want\n0000....\".\n\nI don't see anything wrong with this narrow fix, but I wonder how close\nwe are to just doing something useful here for the user. I.e. if we\ncan't parse_object() what we're about to \"copy\" to e.g. error() on that,\nbut carry on cloning the rest if possible.\n\nWe should probably always return a non-zero at the end, suggesting the\nuser omit the bad ref with a refspec. But if we did all that we could\nusefully recover partially corrupt-repos in this state...\n"},{"id":"454032","messageId":"xmqqczhbo4s3.fsf@gitster.g","threadId":"57755","inReplyTo":"pull.1214.git.1650301959803.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: ignore invalid local refs in remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-20T20:53:32Z","receivedAt":"2022-04-20T20:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n> Subject: Re: [PATCH] clone: ignore invalid local refs in remote\n\nAfter seeing the title, I expected that cloning from such a\nrepository with cruft in .git/refs/ directory would issue a warning\nand succeed without these non-ref files.\n\nBut that is not what is happening here?\n\n> +test_expect_success 'local clone from repo with corrupt refs fails gracefully' '\n> +\tgit init corrupt &&\n> +\ttest_commit -C corrupt one &&\n> +\techo a >corrupt/.git/refs/heads/topic &&\n> +\n> +\ttest_must_fail git clone corrupt working 2>err &&\n> +\tgrep \"has a null OID\" err\n> +'\n> +\n\nWe keep expecting that clone _will_ fail.\n\nSo the net change is that we still do not tolerate a corrupt\nrepository and do not let corruption to propagate through cloning,\nbut we diagnose this breakage as an error by calling die(), which \nis appropriate for dealing with runtime data error, instead of\nhitting a BUG(), which is reserved for program errors.\n\nI agree with the fixed behaviour and implementation.  It just is\nthat \"ignore\" on the title seems misleading.  Other than that,\nthanks for a good finding and a clean fix.\n\n> -\tif (!new_oid || is_null_oid(new_oid))\n> -\t\tBUG(\"create called without valid new_oid\");\n> +\tif (!new_oid || is_null_oid(new_oid)) {\n> +\t\tstrbuf_addf(err, \"'%s' has a null OID\", refname);\n> +\t\treturn 1;\n> +\t}\n"},{"id":"454077","messageId":"b884d627-4a81-5be9-eed5-feff3b2bd010@github.com","threadId":"57755","inReplyTo":"xmqqczhbo4s3.fsf@gitster.g","subject":"Re: [PATCH] clone: ignore invalid local refs in remote","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-21T13:29:32Z","receivedAt":"2022-04-21T13:29:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/20/2022 4:53 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <derrickstolee@github.com>\n>> Subject: Re: [PATCH] clone: ignore invalid local refs in remote\n> \n> After seeing the title, I expected that cloning from such a\n> repository with cruft in .git/refs/ directory would issue a warning\n> and succeed without these non-ref files.\n> \n> But that is not what is happening here?\n\nSorry, this title of the commit message is stale from a version\nwhere I started making the clones succeed (but without these\nbad refs). I changed my mind to only switch BUG() to die() to\navoid giving the impression that we have a \"matching\" repo after\nthe clone.\n \n>> +test_expect_success 'local clone from repo with corrupt refs fails gracefully' '\n>> +\tgit init corrupt &&\n>> +\ttest_commit -C corrupt one &&\n>> +\techo a >corrupt/.git/refs/heads/topic &&\n>> +\n>> +\ttest_must_fail git clone corrupt working 2>err &&\n>> +\tgrep \"has a null OID\" err\n>> +'\n>> +\n> \n> We keep expecting that clone _will_ fail.\n> \n> So the net change is that we still do not tolerate a corrupt\n> repository and do not let corruption to propagate through cloning,\n> but we diagnose this breakage as an error by calling die(), which \n> is appropriate for dealing with runtime data error, instead of\n> hitting a BUG(), which is reserved for program errors.\n> \n> I agree with the fixed behaviour and implementation.  It just is\n> that \"ignore\" on the title seems misleading.  Other than that,\n> thanks for a good finding and a clean fix.\n\nPerhaps the commit could instead start with\n\n\tclone: die() instead of BUG() on bad refs\n\n?\n\nThanks,\n-Stolee\n"},{"id":"454098","messageId":"xmqq7d7ijozz.fsf@gitster.g","threadId":"57755","inReplyTo":"b884d627-4a81-5be9-eed5-feff3b2bd010@github.com","subject":"Re: [PATCH] clone: ignore invalid local refs in remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T18:00:16Z","receivedAt":"2022-04-21T18:00:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> Sorry, this title of the commit message is stale from a version\n> where I started making the clones succeed (but without these\n> bad refs). I changed my mind to only switch BUG() to die() to\n> avoid giving the impression that we have a \"matching\" repo after\n> the clone.\n\nAh, that figures ;-)\n\n> Perhaps the commit could instead start with\n>\n> \tclone: die() instead of BUG() on bad refs\n>\n> ?\n\nYeah, it would be the title I would have been happy with.\n\n\n\n"},{"id":"454383","messageId":"pull.1214.v2.git.1650894450441.gitgitgadget@gmail.com","threadId":"57755","inReplyTo":"pull.1214.git.1650301959803.gitgitgadget@gmail.com","subject":"[PATCH v2] clone: die() instead of BUG() on bad refs","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-25T13:47:30Z","receivedAt":"2022-04-25T13:47:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen cloning directly from a local repository, we load a list of refs\nbased on scanning the $GIT_DIR/refs/ directory of the \"server\"\nrepository. If files exist in that directory that do not parse as\nhexadecimal hashes, then the ref array used by write_remote_refs()\nends up with some entries with null OIDs. This causes us to hit a BUG()\nstatement in ref_transaction_create():\n\n  BUG: create called without valid new_oid\n\nThis BUG() call used to be a die() until 033abf97f (Replace all\ndie(\"BUG: ...\") calls by BUG() ones, 2018-05-02). Before that, the die()\nwas added by f04c5b552 (ref_transaction_create(): check that new_sha1 is\nvalid, 2015-02-17).\n\nThe original report for this bug [1] mentioned that this problem did not\nexist in Git 2.27.0. The failure bisects unsurprisingly to 968f12fda\n(refs: turn on GIT_REF_PARANOIA by default, 2021-09-24). When\nGIT_REF_PARANOIA is enabled, this case always fails as far back as I am\nable to successfully compile and test the Git codebase.\n\n[1] https://github.com/git-for-windows/git/issues/3781\n\nThere are two approaches to consider here. One would be to remove this\nBUG() statement in favor of returning with an error. There are only two\ncallers to ref_transaction_create(), so this would have a limited\nimpact.\n\nThe other approach would be to add special casing in 'git clone' to\navoid this faulty input to the method.\n\nWhile I originally started with changing 'git clone', I decided that\nmodifying ref_transaction_create() was a more complete solution. This\nprevents failing with a BUG() statement when we already have a good way\nto report an error (including a reason for that error) within the\nmethod. Both callers properly check the return value and die() with the\nerror message, so this is an appropriate direction.\n\nThe added test helps check against a regression, but does check that our\nintended error message is handled correctly.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n    clone: ignore invalid local refs in remote\n    \n    Update in v2:\n    \n     * Subject line is improved to better match the message and fix.\n     * I did not explore Ævar's idea to optimistically continue the clone as\n       much as possible. That seems like a bigger change that could be\n       investigated independently.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1214%2Fderrickstolee%2Frefs-bug-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1214/derrickstolee/refs-bug-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1214\n\nRange-diff vs v1:\n\n 1:  6c9e85c5a98 ! 1:  9801123c534 clone: ignore invalid local refs in remote\n     @@ Metadata\n      Author: Derrick Stolee <derrickstolee@github.com>\n      \n       ## Commit message ##\n     -    clone: ignore invalid local refs in remote\n     +    clone: die() instead of BUG() on bad refs\n      \n          When cloning directly from a local repository, we load a list of refs\n          based on scanning the $GIT_DIR/refs/ directory of the \"server\"\n\n\n refs.c                 | 6 ++++--\n t/t5605-clone-local.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1a964505f92..f300f83e4d4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1111,8 +1111,10 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n-\tif (!new_oid || is_null_oid(new_oid))\n-\t\tBUG(\"create called without valid new_oid\");\n+\tif (!new_oid || is_null_oid(new_oid)) {\n+\t\tstrbuf_addf(err, \"'%s' has a null OID\", refname);\n+\t\treturn 1;\n+\t}\n \treturn ref_transaction_update(transaction, refname, new_oid,\n \t\t\t\t      null_oid(), flags, msg, err);\n }\ndiff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh\nindex 7d63365f93a..21ab6192839 100755\n--- a/t/t5605-clone-local.sh\n+++ b/t/t5605-clone-local.sh\n@@ -141,4 +141,13 @@ test_expect_success 'cloning locally respects \"-u\" for fetching refs' '\n \ttest_must_fail git clone --bare -u false a should_not_work.git\n '\n \n+test_expect_success 'local clone from repo with corrupt refs fails gracefully' '\n+\tgit init corrupt &&\n+\ttest_commit -C corrupt one &&\n+\techo a >corrupt/.git/refs/heads/topic &&\n+\n+\ttest_must_fail git clone corrupt working 2>err &&\n+\tgrep \"has a null OID\" err\n+'\n+\n test_done\n\nbase-commit: 11cfe552610386954886543f5de87dcc49ad5735\n-- \ngitgitgadget\n"}]}