{"thread":{"id":"54426","subject":"[PATCH] fetch-pack: show detailed error in read_pack_header","startedAt":"2020-10-15T11:42:13Z","lastAt":"2020-10-26T17:41:00Z","messageCount":2,"participants":["Nipunn Koorapati via GitGitGadget","Nipunn Koorapati"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407593","messageId":"pull.755.git.1602762128039.gitgitgadget@gmail.com","threadId":"54426","inReplyTo":null,"subject":"[PATCH] fetch-pack: show detailed error in read_pack_header","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T11:42:07Z","receivedAt":"2020-10-15T11:42:13Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nWhen fetch-pack fails with a bad pack header, the\nprovided error globs together several distinct error types -\nEOF, Bad Signature, and Pack version unsupported.\n\nProvide the more detailed error\nto the user so they can debug their situation further.\n\nBefore:\nprotocol error: bad pack header\n\nAfter:\nprotocol error: bad pack header: eof before pack header was fully read\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n    [fetch-pack] Show detailed error in read_pack_header\n    \n    I saw the \"bad pack header\" error when using partial clone on v2.28.0\n    without allowanysha1inwant flag, but the error failed to include detail\n    of why the header was bad. The error message no longer occurs on\n    v2.29.0-rc1. Details here\n    https://public-inbox.org/git/CAN8Z4-XgctFZxZoTWRpD1V9NFr34ObzG2dxUoAfuJ4NOsBDdtg@mail.gmail.com/\n    \n    I based my change off of v2.28.0 - writing a test case for the error I\n    saw. The test case no longer passes on master - so I removed it from the\n    patch - but am including it here in the cover letter as an illustration\n    of what is being fixed.\n    \n    --- a/t/t5616-partial-clone.sh\n    +++ b/t/t5616-partial-clone.sh\n    @@ -25,7 +25,18 @@ test_expect_success 'setup normal src repo' '\n     # bare clone \"src\" giving \"srv.bare\" for use as our server.\n     test_expect_success 'setup bare clone for server' '\n         git clone --bare \"file://$(pwd)/src\" srv.bare &&\n    -    git -C srv.bare config --local uploadpack.allowfilter 1 &&\n    +    git -C srv.bare config --local uploadpack.allowfilter 1\n    +'\n    +\n    +# Confirm that partial cloning fails with error when\n    +# allowanysha1inwant is not set. Expect checkout to fail\n    +# after clone succeeds\n    +#\n    +# Then set for rest of tests\n    +test_expect_success 'error on partial clone when allowanysha1inwant not set' '\n    +    test_must_fail git clone --filter=blob:none \"file://$(pwd)/srv.bare\" pc1 2>err &&\n    +    test_i18ngrep \"fatal: protocol error: bad pack header: eof before pack header was fully read\" err &&\n    +    rm -rf pc1 &&\n         git -C srv.bare config --local uploadpack.allowanysha1inwant 1\n     '\n    \n    Thank you! Nipunn\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-755%2Fnipunn1313%2Ferror_msg_off_2.28-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-755/nipunn1313/error_msg_off_2.28-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/755\n\n builtin/receive-pack.c | 22 +---------------------\n fetch-pack.c           |  6 ++++--\n pack.h                 |  1 +\n sha1-file.c            | 21 +++++++++++++++++++++\n 4 files changed, 27 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bb9909c52e..c1b572cf7d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2100,26 +2100,6 @@ static void read_push_options(struct packet_reader *reader,\n \t}\n }\n \n-static const char *parse_pack_header(struct pack_header *hdr)\n-{\n-\tswitch (read_pack_header(0, hdr)) {\n-\tcase PH_ERROR_EOF:\n-\t\treturn \"eof before pack header was fully read\";\n-\n-\tcase PH_ERROR_PACK_SIGNATURE:\n-\t\treturn \"protocol error (pack signature mismatch detected)\";\n-\n-\tcase PH_ERROR_PROTOCOL:\n-\t\treturn \"protocol error (pack version unsupported)\";\n-\n-\tdefault:\n-\t\treturn \"unknown error in parse_pack_header\";\n-\n-\tcase 0:\n-\t\treturn NULL;\n-\t}\n-}\n-\n static const char *pack_lockfile;\n \n static void push_header_arg(struct strvec *args, struct pack_header *hdr)\n@@ -2140,7 +2120,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\t    ? transfer_fsck_objects\n \t\t\t    : 0);\n \n-\thdr_err = parse_pack_header(&hdr);\n+\thdr_err = pack_header_error(read_pack_header(0, &hdr));\n \tif (hdr_err) {\n \t\tif (err_fd > 0)\n \t\t\tclose(err_fd);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b10c432315..ad9db33e17 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -809,6 +809,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tint pass_header = 0;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint ret;\n+\tconst char *ph_error;\n \n \tmemset(&demux, 0, sizeof(demux));\n \tif (use_sideband) {\n@@ -828,8 +829,9 @@ static int get_pack(struct fetch_pack_args *args,\n \n \tif (!args->keep_pack && unpack_limit) {\n \n-\t\tif (read_pack_header(demux.out, &header))\n-\t\t\tdie(_(\"protocol error: bad pack header\"));\n+\t\tph_error = pack_header_error(read_pack_header(demux.out, &header));\n+\t\tif (ph_error)\n+\t\t\tdie(_(\"protocol error: bad pack header: %s\"), ph_error);\n \t\tpass_header = 1;\n \t\tif (ntohl(header.hdr_entries) < unpack_limit)\n \t\t\tdo_keep = 0;\ndiff --git a/pack.h b/pack.h\nindex 9fc0945ac9..63d060d5c2 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -99,6 +99,7 @@ int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,\n #define PH_ERROR_PACK_SIGNATURE\t(-2)\n #define PH_ERROR_PROTOCOL\t(-3)\n int read_pack_header(int fd, struct pack_header *);\n+const char *pack_header_error(int ph_err);\n \n struct hashfile *create_tmp_packfile(char **pack_tmp_name);\n void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex dd65bd5c68..d711096054 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -2252,6 +2252,27 @@ int read_pack_header(int fd, struct pack_header *header)\n \treturn 0;\n }\n \n+const char *pack_header_error(int err)\n+{\n+\tswitch (err) {\n+\tcase PH_ERROR_EOF:\n+\t\treturn \"eof before pack header was fully read\";\n+\n+\tcase PH_ERROR_PACK_SIGNATURE:\n+\t\treturn \"protocol error (pack signature mismatch detected)\";\n+\n+\tcase PH_ERROR_PROTOCOL:\n+\t\treturn \"protocol error (pack version unsupported)\";\n+\n+\tdefault:\n+\t\t// Should not occur - all errors should be handled\n+\t\tdie(\"unknown error in parse_pack_header %d\", err);\n+\n+\tcase 0:\n+\t\treturn NULL;\n+\t}\n+}\n+\n void assert_oid_type(const struct object_id *oid, enum object_type expect)\n {\n \tenum object_type type = oid_object_info(the_repository, oid, NULL);\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"408426","messageId":"CAN8Z4-XT91FJY8sWnEp1D3kXjhrc_y9VT3LrrEwBZadmKfVGGw@mail.gmail.com","threadId":"54426","inReplyTo":"pull.755.git.1602762128039.gitgitgadget@gmail.com","subject":"Re: [PATCH] fetch-pack: show detailed error in read_pack_header","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-26T17:40:46Z","receivedAt":"2020-10-26T17:41:00Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Hi - wanted to bump this to see what folks think about this error\nmessage improvement.\nIt was helpful to me while debugging - and seems fairly non-risky. I'm\nhappy to continue\npursuing this, or to drop it, depending on mailing list conversation.\n"}]}