{"thread":{"id":"11759","subject":"[RFC] Don't expect verify_pack() callers to set pack_size","startedAt":"2008-01-27T15:01:38Z","lastAt":"2008-01-27T17:18:26Z","messageCount":2,"participants":["Mike Hommey","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"66676","messageId":"1201446098-18868-1-git-send-email-mh@glandium.org","threadId":"11759","inReplyTo":null,"subject":"[RFC] Don't expect verify_pack() callers to set pack_size","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-01-27T15:01:38Z","receivedAt":"2008-01-27T15:01:38Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Since use_pack() will end up populating pack_size if it is not already set,\nwe can just adapt the code in verify_packfile() such that it doesn't require\npack_size to be set beforehand.\n\nThis allows callers not to have to set pack_size themselves, and we can thus\nrevert changes from 1c23d794.\n---\n\n I'd like some feedback on this change in pack-check.c, that help refactoring\n more efficiently the code in http-*.c.\n\n http-push.c   |    3 ---\n http-walker.c |    1 -\n pack-check.c  |    8 +++++---\n 3 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex b2b410d..be54ed3 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -770,14 +770,11 @@ static void finish_request(struct transfer_request *request)\n \t\t\t\trequest->url, curl_errorstr);\n \t\t\tremote->can_update_info_refs = 0;\n \t\t} else {\n-\t\t\toff_t pack_size = ftell(request->local_stream);\n-\n \t\t\tfclose(request->local_stream);\n \t\t\trequest->local_stream = NULL;\n \t\t\tif (!move_temp_to_file(request->tmpfile,\n \t\t\t\t\t       request->filename)) {\n \t\t\t\ttarget = (struct packed_git *)request->userData;\n-\t\t\t\ttarget->pack_size = pack_size;\n \t\t\t\tlst = &remote->packs;\n \t\t\t\twhile (*lst != target)\n \t\t\t\t\tlst = &((*lst)->next);\ndiff --git a/http-walker.c b/http-walker.c\nindex 2c37868..29f9728 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -783,7 +783,6 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha\n \t\treturn error(\"Unable to start request\");\n \t}\n \n-\ttarget->pack_size = ftell(packfile);\n \tfclose(packfile);\n \n \tret = move_temp_to_file(tmpfile, filename);\ndiff --git a/pack-check.c b/pack-check.c\nindex d7dd62b..e7f0126 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -25,7 +25,7 @@ static int verify_packfile(struct packed_git *p,\n \tconst unsigned char *index_base = p->index_data;\n \tSHA_CTX ctx;\n \tunsigned char sha1[20];\n-\toff_t offset = 0, pack_sig = p->pack_size - 20;\n+\toff_t offset = 0, pack_sig = 0;\n \tuint32_t nr_objects, i;\n \tint err;\n \tstruct idx_entry *entries;\n@@ -37,14 +37,16 @@ static int verify_packfile(struct packed_git *p,\n \t */\n \n \tSHA1_Init(&ctx);\n-\twhile (offset < pack_sig) {\n+\tdo {\n \t\tunsigned int remaining;\n \t\tunsigned char *in = use_pack(p, w_curs, offset, &remaining);\n \t\toffset += remaining;\n+\t\tif (pack_sig == 0)\n+\t\t\tpack_sig = p->pack_size - 20;\n \t\tif (offset > pack_sig)\n \t\t\tremaining -= (unsigned int)(offset - pack_sig);\n \t\tSHA1_Update(&ctx, in, remaining);\n-\t}\n+\t} while (offset < pack_sig);\n \tSHA1_Final(sha1, &ctx);\n \tif (hashcmp(sha1, use_pack(p, w_curs, pack_sig, NULL)))\n \t\treturn error(\"Packfile %s SHA1 mismatch with itself\",\n-- \n1.5.4.rc5.12.gb8680-dirty\n"},{"id":"66678","messageId":"20080127171826.GV24004@spearce.org","threadId":"11759","inReplyTo":"1201446098-18868-1-git-send-email-mh@glandium.org","subject":"Re: [RFC] Don't expect verify_pack() callers to set pack_size","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-01-27T17:18:26Z","receivedAt":"2008-01-27T17:18:26Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Mike Hommey <mh@glandium.org> wrote:\n> Since use_pack() will end up populating pack_size if it is not already set,\n> we can just adapt the code in verify_packfile() such that it doesn't require\n> pack_size to be set beforehand.\n> \n> This allows callers not to have to set pack_size themselves, and we can thus\n> revert changes from 1c23d794.\n\nThat's a good improvement in code clarity.  When I implemented\n1c23 I wasn't sure what was initialized in the struct packed_git\nthat the HTTP code was slugging around in request->userData, and\nit was hard to track down.  1c23 was a quick fix to get HTTP fetch\nworking again when sp/mmap merged into next.\n\nI'm guessing you have figured out it is not actually open at this\npoint, which makes sense as we did fclose() calls to close out\nthe writer.  So sha1_file.c needs to open a new file descriptor.\nuse_pack() will force it to be opened and the open packfile function\ndoes update the pack_size if we don't have it yet.\n\nAcked-by: Shawn O. Pearce <spearce@spearce.org>\n\n> diff --git a/pack-check.c b/pack-check.c\n> index d7dd62b..e7f0126 100644\n> --- a/pack-check.c\n> +++ b/pack-check.c\n> @@ -37,14 +37,16 @@ static int verify_packfile(struct packed_git *p,\n>  \t */\n>  \n>  \tSHA1_Init(&ctx);\n> -\twhile (offset < pack_sig) {\n> +\tdo {\n>  \t\tunsigned int remaining;\n>  \t\tunsigned char *in = use_pack(p, w_curs, offset, &remaining);\n>  \t\toffset += remaining;\n> +\t\tif (pack_sig == 0)\n> +\t\t\tpack_sig = p->pack_size - 20;\n\nfyi, we prefer \"if (!pack_sig)\" in Git for equality with 0 tests.\n\n-- \nShawn.\n"}]}