{"thread":{"id":"50997","subject":"[PATCH 0/1] commit-graph: improve error messages","startedAt":"2019-04-26T11:48:29Z","lastAt":"2019-04-26T12:33:13Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"374554","messageId":"pull.181.git.gitgitgadget@gmail.com","threadId":"50997","inReplyTo":null,"subject":"[PATCH 0/1] commit-graph: improve error messages","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-26T11:48:24Z","receivedAt":"2019-04-26T11:48:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is a small patch that revises the error messages from\nab/commit-graph-fixes, as recommended by Ævar. Hopefully, it can be merged\nfaster than the commit-graph v2 stuff, and I can update that series to\ninclude this change if we agree it is a good one.\n\nThanks, -Stolee\n\nCc: avarab@gmail.com\n\nIn-Reply-To: 878svxrwsh.fsf@evledraar.gmail.com\n[878svxrwsh.fsf@evledraar.gmail.com]\n\nDerrick Stolee (1):\n  commit-graph: improve error messages\n\n commit-graph.c          | 10 +++++-----\n t/t5318-commit-graph.sh |  6 +++---\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\n\nbase-commit: 93b4405ffe4ad9308740e7c1c71383bfc369baaa\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-181%2Fderrickstolee%2Fgraph%2Fmessage-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-181/derrickstolee/graph/message-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/181\n-- \ngitgitgadget\n"},{"id":"374555","messageId":"1751b479e79ba18990e4152ae2acdf60c8713340.1556279303.git.gitgitgadget@gmail.com","threadId":"50997","inReplyTo":"pull.181.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] commit-graph: improve error messages","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-26T11:48:25Z","receivedAt":"2019-04-26T11:48:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe error messages when reading a commit-graph have a few problems:\n\n1. Some values are output in hexadecimal, but that is not made\n   clear by the message. Prepend \"0x\" to these values.\n\n2. The version number does not need to be hexadecimal, and also\n   should mention a \"maximum supported version\". This has one\n   possible confusing case: we could have a corrupt commit-graph\n   file with version number zero, so the output would be\n\n   \"commit-graph has version 0, our maximum supported version is 1\"\n\n   This will only happen with corrupt data. This error message is\n   designed instead for the case where a client is downgraded after\n   writing a newer file version.\n\nUpdate t5318-commit-graph.sh to watch for these new error messages.\n\nReported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n commit-graph.c          | 10 +++++-----\n t/t5318-commit-graph.sh |  6 +++---\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 66865acbd7..aba591913e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -167,21 +167,21 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \n \tgraph_signature = get_be32(data);\n \tif (graph_signature != GRAPH_SIGNATURE) {\n-\t\terror(_(\"commit-graph signature %X does not match signature %X\"),\n+\t\terror(_(\"commit-graph signature 0x%X does not match signature 0x%X\"),\n \t\t      graph_signature, GRAPH_SIGNATURE);\n \t\treturn NULL;\n \t}\n \n \tgraph_version = *(unsigned char*)(data + 4);\n \tif (graph_version != GRAPH_VERSION) {\n-\t\terror(_(\"commit-graph version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph has version %d, our maximum supported version is %d\"),\n \t\t      graph_version, GRAPH_VERSION);\n \t\treturn NULL;\n \t}\n \n \thash_version = *(unsigned char*)(data + 5);\n \tif (hash_version != oid_version()) {\n-\t\terror(_(\"commit-graph hash version %X does not match version %X\"),\n+\t\terror(_(\"commit-graph has hash version %d, our maximum supported version is %d\"),\n \t\t      hash_version, oid_version());\n \t\treturn NULL;\n \t}\n@@ -215,7 +215,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tchunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;\n \n \t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n-\t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n+\t\t\terror(_(\"commit-graph improper chunk offset 0x%08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t      (uint32_t)chunk_offset);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n@@ -252,7 +252,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\t}\n \n \t\tif (chunk_repeated) {\n-\t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n+\t\t\terror(_(\"commit-graph chunk id 0x%08x appears multiple times\"), chunk_id);\n \t\t\tfree(graph);\n \t\t\treturn NULL;\n \t\t}\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex e80c1cac02..264ebb15b1 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -420,17 +420,17 @@ test_expect_success 'detect too small' '\n \n test_expect_success 'detect bad signature' '\n \tcorrupt_graph_and_verify 0 \"\\0\" \\\n-\t\t\"graph signature\"\n+\t\t\"commit-graph signature\"\n '\n \n test_expect_success 'detect bad version' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_VERSION \"\\02\" \\\n-\t\t\"graph version\"\n+\t\t\"commit-graph has version\"\n '\n \n test_expect_success 'detect bad hash version' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_HASH \"\\02\" \\\n-\t\t\"hash version\"\n+\t\t\"commit-graph has hash version\"\n '\n \n test_expect_success 'detect low chunk count' '\n-- \ngitgitgadget\n"},{"id":"374557","messageId":"875zr1rlob.fsf@evledraar.gmail.com","threadId":"50997","inReplyTo":"1751b479e79ba18990e4152ae2acdf60c8713340.1556279303.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: improve error messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-26T12:33:08Z","receivedAt":"2019-04-26T12:33:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 26 2019, Derrick Stolee via GitGitGadget wrote:\n\n> The error messages when reading a commit-graph have a few problems:\n>\n> 1. Some values are output in hexadecimal, but that is not made\n>    clear by the message. Prepend \"0x\" to these values.\n>\n> 2. The version number does not need to be hexadecimal, and also\n>    should mention a \"maximum supported version\". This has one\n>    possible confusing case: we could have a corrupt commit-graph\n>    file with version number zero, so the output would be\n>\n>    \"commit-graph has version 0, our maximum supported version is 1\"\n>\n>    This will only happen with corrupt data. This error message is\n>    designed instead for the case where a client is downgraded after\n>    writing a newer file version.\n\nThis looks good to me and is obviously correct:\n\n> Update t5318-commit-graph.sh to watch for these new error messages.\n> [...]\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index e80c1cac02..264ebb15b1 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -420,17 +420,17 @@ test_expect_success 'detect too small' '\n>\n>  test_expect_success 'detect bad signature' '\n>  \tcorrupt_graph_and_verify 0 \"\\0\" \\\n> -\t\t\"graph signature\"\n> +\t\t\"commit-graph signature\"\n>  '\n>\n>  test_expect_success 'detect bad version' '\n>  \tcorrupt_graph_and_verify $GRAPH_BYTE_VERSION \"\\02\" \\\n> -\t\t\"graph version\"\n> +\t\t\"commit-graph has version\"\n>  '\n>\n>  test_expect_success 'detect bad hash version' '\n>  \tcorrupt_graph_and_verify $GRAPH_BYTE_HASH \"\\02\" \\\n> -\t\t\"hash version\"\n> +\t\t\"commit-graph has hash version\"\n>  '\n>\n>  test_expect_success 'detect low chunk count' '\n\nJust a small nit/OCD :).\n\nThe only change to that test code that's actually needed is just doing:\n\n    -               \"graph version\"\n    +               \"graph has version\"\n\nThe rest were already grepping subsets of the message before/after, and\nmaybe worth it to keep it consistent that all these\n\"corrupt_graph_and_verify\" invocations grep the part of the message that\nisn't the \"commit-graph <whatever>\", i.e. now with just that change:\n\n    $ git grep -h -A1 corrupt_graph_and_verify|grep -o '\".*\"$'\n    \"graph signature\"\n    \"graph has version\"\n    \"hash version\"\n    \"missing the .* chunk\"\n    \"missing the OID Fanout chunk\"\n    \"missing the OID Lookup chunk\"\n    \"missing the Commit Data chunk\"\n    \"fanout value\"\n    \"fanout value\"\n    \"incorrect OID order\"\n    \"from object database\"\n    \"root tree OID for commit\"\n    \"invalid parent\"\n    \"is too long\"\n    \"commit-graph parent for\"\n    \"generation for commit\"\n    \"non-zero generation number\"\n    \"commit date\"\n    \"invalid parent\"\n    \"incorrect checksum\"\n\nWell, there's the sore thumb of \"commit-graph parent for\".\n"}]}