diff mbox series

fetch-pack: show detailed error in read_pack_header

Message ID pull.755.git.1602762128039.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series fetch-pack: show detailed error in read_pack_header | expand

Commit Message

Philippe Blain via GitGitGadget Oct. 15, 2020, 11:42 a.m. UTC 
From: Nipunn Koorapati <nipunn@dropbox.com>

When fetch-pack fails with a bad pack header, the
provided error globs together several distinct error types -
EOF, Bad Signature, and Pack version unsupported.

Provide the more detailed error
to the user so they can debug their situation further.

Before:
protocol error: bad pack header

After:
protocol error: bad pack header: eof before pack header was fully read

Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
---
    [fetch-pack] Show detailed error in read_pack_header
    
    I saw the "bad pack header" error when using partial clone on v2.28.0
    without allowanysha1inwant flag, but the error failed to include detail
    of why the header was bad. The error message no longer occurs on
    v2.29.0-rc1. Details here
    https://public-inbox.org/git/CAN8Z4-XgctFZxZoTWRpD1V9NFr34ObzG2dxUoAfuJ4NOsBDdtg@mail.gmail.com/
    
    I based my change off of v2.28.0 - writing a test case for the error I
    saw. The test case no longer passes on master - so I removed it from the
    patch - but am including it here in the cover letter as an illustration
    of what is being fixed.
    
    --- a/t/t5616-partial-clone.sh
    +++ b/t/t5616-partial-clone.sh
    @@ -25,7 +25,18 @@ test_expect_success 'setup normal src repo' '
     # bare clone "src" giving "srv.bare" for use as our server.
     test_expect_success 'setup bare clone for server' '
         git clone --bare "file://$(pwd)/src" srv.bare &&
    -    git -C srv.bare config --local uploadpack.allowfilter 1 &&
    +    git -C srv.bare config --local uploadpack.allowfilter 1
    +'
    +
    +# Confirm that partial cloning fails with error when
    +# allowanysha1inwant is not set. Expect checkout to fail
    +# after clone succeeds
    +#
    +# Then set for rest of tests
    +test_expect_success 'error on partial clone when allowanysha1inwant not set' '
    +    test_must_fail git clone --filter=blob:none "file://$(pwd)/srv.bare" pc1 2>err &&
    +    test_i18ngrep "fatal: protocol error: bad pack header: eof before pack header was fully read" err &&
    +    rm -rf pc1 &&
         git -C srv.bare config --local uploadpack.allowanysha1inwant 1
     '
    
    Thank you! Nipunn

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-755%2Fnipunn1313%2Ferror_msg_off_2.28-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-755/nipunn1313/error_msg_off_2.28-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/755

 builtin/receive-pack.c | 22 +---------------------
 fetch-pack.c           |  6 ++++--
 pack.h                 |  1 +
 sha1-file.c            | 21 +++++++++++++++++++++
 4 files changed, 27 insertions(+), 23 deletions(-)


base-commit: d4a392452e292ff924e79ec8458611c0f679d6d4

Comments

Nipunn Koorapati Oct. 26, 2020, 5:40 p.m. UTC | #1 
Hi - wanted to bump this to see what folks think about this error
message improvement.
It was helpful to me while debugging - and seems fairly non-risky. I'm
happy to continue
pursuing this, or to drop it, depending on mailing list conversation.
diff mbox series

Patch

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..c1b572cf7d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2100,26 +2100,6 @@  static void read_push_options(struct packet_reader *reader,
 	}
 }
 
-static const char *parse_pack_header(struct pack_header *hdr)
-{
-	switch (read_pack_header(0, hdr)) {
-	case PH_ERROR_EOF:
-		return "eof before pack header was fully read";
-
-	case PH_ERROR_PACK_SIGNATURE:
-		return "protocol error (pack signature mismatch detected)";
-
-	case PH_ERROR_PROTOCOL:
-		return "protocol error (pack version unsupported)";
-
-	default:
-		return "unknown error in parse_pack_header";
-
-	case 0:
-		return NULL;
-	}
-}
-
 static const char *pack_lockfile;
 
 static void push_header_arg(struct strvec *args, struct pack_header *hdr)
@@ -2140,7 +2120,7 @@  static const char *unpack(int err_fd, struct shallow_info *si)
 			    ? transfer_fsck_objects
 			    : 0);
 
-	hdr_err = parse_pack_header(&hdr);
+	hdr_err = pack_header_error(read_pack_header(0, &hdr));
 	if (hdr_err) {
 		if (err_fd > 0)
 			close(err_fd);
diff --git a/fetch-pack.c b/fetch-pack.c
index b10c432315..ad9db33e17 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -809,6 +809,7 @@  static int get_pack(struct fetch_pack_args *args,
 	int pass_header = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	int ret;
+	const char *ph_error;
 
 	memset(&demux, 0, sizeof(demux));
 	if (use_sideband) {
@@ -828,8 +829,9 @@  static int get_pack(struct fetch_pack_args *args,
 
 	if (!args->keep_pack && unpack_limit) {
 
-		if (read_pack_header(demux.out, &header))
-			die(_("protocol error: bad pack header"));
+		ph_error = pack_header_error(read_pack_header(demux.out, &header));
+		if (ph_error)
+			die(_("protocol error: bad pack header: %s"), ph_error);
 		pass_header = 1;
 		if (ntohl(header.hdr_entries) < unpack_limit)
 			do_keep = 0;
diff --git a/pack.h b/pack.h
index 9fc0945ac9..63d060d5c2 100644
--- a/pack.h
+++ b/pack.h
@@ -99,6 +99,7 @@  int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,
 #define PH_ERROR_PACK_SIGNATURE	(-2)
 #define PH_ERROR_PROTOCOL	(-3)
 int read_pack_header(int fd, struct pack_header *);
+const char *pack_header_error(int ph_err);
 
 struct hashfile *create_tmp_packfile(char **pack_tmp_name);
 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[]);
diff --git a/sha1-file.c b/sha1-file.c
index dd65bd5c68..d711096054 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -2252,6 +2252,27 @@  int read_pack_header(int fd, struct pack_header *header)
 	return 0;
 }
 
+const char *pack_header_error(int err)
+{
+	switch (err) {
+	case PH_ERROR_EOF:
+		return "eof before pack header was fully read";
+
+	case PH_ERROR_PACK_SIGNATURE:
+		return "protocol error (pack signature mismatch detected)";
+
+	case PH_ERROR_PROTOCOL:
+		return "protocol error (pack version unsupported)";
+
+	default:
+		// Should not occur - all errors should be handled
+		die("unknown error in parse_pack_header %d", err);
+
+	case 0:
+		return NULL;
+	}
+}
+
 void assert_oid_type(const struct object_id *oid, enum object_type expect)
 {
 	enum object_type type = oid_object_info(the_repository, oid, NULL);