{"thread":{"id":"5630","subject":"[PATCH 2/6] many cleanups to sha1_file.c","startedAt":"2006-09-21T04:05:37Z","lastAt":"2006-09-24T04:17:16Z","messageCount":3,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"27328","messageId":"Pine.LNX.4.64.0609210004550.2627@xanadu.home","threadId":"5630","inReplyTo":null,"subject":"[PATCH 2/6] many cleanups to sha1_file.c","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-09-21T04:05:37Z","receivedAt":"2006-09-21T04:05:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Those cleanups are mainly to set the table for the support of deltas\nwith base objects referenced by offsets instead of sha1.  This means\nthat many pack lookup functions are converted to take a pack/offset\ntuple instead of a sha1.\n\nThis eliminates many struct pack_entry usages since this structure\ncarried redundent information in many cases, and it increased stack\nfootprint needlessly for a couple recursively called functions that used\nto declare a local copy of it for every recursion loop.\n\nIn the process, packed_object_info_detail() has been reorganized as well\nso to look much saner and more amenable to deltas with offset support.\n\nFinally the appropriate adjustments have been made to functions that\ndepend on the above changes.  But there is no functionality changes yet\nsimply some code refactoring at this point.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n\n---\n\nThis patch might look a bit frightening since it touches core code quite\nextensively but all those changes must occur at the same time to keep the\ncode fully functional.  All tests are still passing fine of course.  And\nagain there should not be any change in behavior with this patch.\n\nThis will allow for the new functionalities to be added with a minimum of\nchanges later on and therefore easier to evaluate.\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 8d7a120..96c069a 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -597,15 +597,15 @@ static int add_object_entry(const unsign\n \n \tif (!exclude) {\n \t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\tstruct pack_entry e;\n-\t\t\tif (find_pack_entry_one(sha1, &e, p)) {\n+\t\t\tunsigned long offset = find_pack_entry_one(sha1, p);\n+\t\t\tif (offset) {\n \t\t\t\tif (incremental)\n \t\t\t\t\treturn 0;\n \t\t\t\tif (local && !p->pack_local)\n \t\t\t\t\treturn 0;\n \t\t\t\tif (!found_pack) {\n-\t\t\t\t\tfound_offset = e.offset;\n-\t\t\t\t\tfound_pack = e.p;\n+\t\t\t\t\tfound_offset = offset;\n+\t\t\t\t\tfound_pack = p;\n \t\t\t\t}\n \t\t\t}\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex 282eed6..42f13e2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -383,10 +383,10 @@ extern void unuse_packed_git(struct pack\n extern struct packed_git *add_packed_git(char *, int, int);\n extern int num_packed_objects(const struct packed_git *p);\n extern int nth_packed_object_sha1(const struct packed_git *, int, unsigned char*);\n-extern int find_pack_entry_one(const unsigned char *, struct pack_entry *, struct packed_git *);\n-extern void *unpack_entry_gently(struct pack_entry *, char *, unsigned long *);\n+extern unsigned long find_pack_entry_one(const unsigned char *, struct packed_git *);\n+extern void *unpack_entry_gently(struct packed_git *, unsigned long, char *, unsigned long *);\n extern unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n-extern void packed_object_info_detail(struct pack_entry *, char *, unsigned long *, unsigned long *, unsigned int *, unsigned char *);\n+extern void packed_object_info_detail(struct packed_git *, unsigned long, char *, unsigned long *, unsigned long *, unsigned int *, unsigned char *);\n \n /* Dumb servers support */\n extern int update_server_info(int);\ndiff --git a/pack-check.c b/pack-check.c\nindex 04c6c00..c0caaee 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -42,16 +42,16 @@ static int verify_packfile(struct packed\n \t */\n \tfor (i = err = 0; i < nr_objects; i++) {\n \t\tunsigned char sha1[20];\n-\t\tstruct pack_entry e;\n \t\tvoid *data;\n \t\tchar type[20];\n-\t\tunsigned long size;\n+\t\tunsigned long size, offset;\n \n \t\tif (nth_packed_object_sha1(p, i, sha1))\n \t\t\tdie(\"internal error pack-check nth-packed-object\");\n-\t\tif (!find_pack_entry_one(sha1, &e, p))\n+\t\toffset = find_pack_entry_one(sha1, p);\n+\t\tif (!offset)\n \t\t\tdie(\"internal error pack-check find-pack-entry-one\");\n-\t\tdata = unpack_entry_gently(&e, type, &size);\n+\t\tdata = unpack_entry_gently(p, offset, type, &size);\n \t\tif (!data) {\n \t\t\terr = error(\"cannot unpack %s from %s\",\n \t\t\t\t    sha1_to_hex(sha1), p->pack_name);\n@@ -84,25 +84,26 @@ static void show_pack_info(struct packed\n \n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunsigned char sha1[20], base_sha1[20];\n-\t\tstruct pack_entry e;\n \t\tchar type[20];\n \t\tunsigned long size;\n \t\tunsigned long store_size;\n+\t\tunsigned long offset;\n \t\tunsigned int delta_chain_length;\n \n \t\tif (nth_packed_object_sha1(p, i, sha1))\n \t\t\tdie(\"internal error pack-check nth-packed-object\");\n-\t\tif (!find_pack_entry_one(sha1, &e, p))\n+\t\toffset = find_pack_entry_one(sha1, p);\n+\t\tif (!offset)\n \t\t\tdie(\"internal error pack-check find-pack-entry-one\");\n \n-\t\tpacked_object_info_detail(&e, type, &size, &store_size,\n+\t\tpacked_object_info_detail(p, offset, type, &size, &store_size,\n \t\t\t\t\t  &delta_chain_length,\n \t\t\t\t\t  base_sha1);\n \t\tprintf(\"%s \", sha1_to_hex(sha1));\n \t\tif (!delta_chain_length)\n-\t\t\tprintf(\"%-6s %lu %u\\n\", type, size, e.offset);\n+\t\t\tprintf(\"%-6s %lu %lu\\n\", type, size, offset);\n \t\telse {\n-\t\t\tprintf(\"%-6s %lu %u %u %s\\n\", type, size, e.offset,\n+\t\t\tprintf(\"%-6s %lu %lu %u %s\\n\", type, size, offset,\n \t\t\t       delta_chain_length, sha1_to_hex(base_sha1));\n \t\t\tif (delta_chain_length < MAX_CHAIN)\n \t\t\t\tchain_histogram[delta_chain_length]++;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b89edb9..6fae766 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -888,33 +888,32 @@ void * unpack_sha1_file(void *map, unsig\n }\n \n /* forward declaration for a mutually recursive function */\n-static int packed_object_info(struct pack_entry *entry,\n+static int packed_object_info(struct packed_git *p, unsigned long offset,\n \t\t\t      char *type, unsigned long *sizep);\n \n-static int packed_delta_info(unsigned char *base_sha1,\n-\t\t\t     unsigned long delta_size,\n-\t\t\t     unsigned long left,\n+static int packed_delta_info(struct packed_git *p,\n+\t\t\t     unsigned long offset,\n \t\t\t     char *type,\n-\t\t\t     unsigned long *sizep,\n-\t\t\t     struct packed_git *p)\n+\t\t\t     unsigned long *sizep)\n {\n-\tstruct pack_entry base_ent;\n+\tunsigned long base_offset;\n+\tunsigned char *base_sha1 = (unsigned char *) p->pack_base + offset;\n \n-\tif (left < 20)\n+\tif (p->pack_size < offset + 20)\n \t\tdie(\"truncated pack file\");\n-\n \t/* The base entry _must_ be in the same pack */\n-\tif (!find_pack_entry_one(base_sha1, &base_ent, p))\n+\tbase_offset = find_pack_entry_one(base_sha1, p);\n+\tif (!base_offset)\n \t\tdie(\"failed to find delta-pack base object %s\",\n \t\t    sha1_to_hex(base_sha1));\n+\toffset += 20;\n \n \t/* We choose to only get the type of the base object and\n \t * ignore potentially corrupt pack file that expects the delta\n \t * based on a base with a wrong size.  This saves tons of\n \t * inflate() calls.\n \t */\n-\n-\tif (packed_object_info(&base_ent, type, NULL))\n+\tif (packed_object_info(p, base_offset, type, NULL))\n \t\tdie(\"cannot get info for delta-pack base\");\n \n \tif (sizep) {\n@@ -926,8 +925,8 @@ static int packed_delta_info(unsigned ch\n \n \t\tmemset(&stream, 0, sizeof(stream));\n \n-\t\tdata = stream.next_in = base_sha1 + 20;\n-\t\tstream.avail_in = left - 20;\n+\t\tdata = stream.next_in = (unsigned char *) p->pack_base + offset;;\n+\t\tstream.avail_in = p->pack_size - offset;\n \t\tstream.next_out = delta_head;\n \t\tstream.avail_out = sizeof(delta_head);\n \n@@ -989,75 +988,60 @@ int check_reuse_pack_delta(struct packed\n \treturn status;\n }\n \n-void packed_object_info_detail(struct pack_entry *e,\n+void packed_object_info_detail(struct packed_git *p,\n+\t\t\t       unsigned long offset,\n \t\t\t       char *type,\n \t\t\t       unsigned long *size,\n \t\t\t       unsigned long *store_size,\n \t\t\t       unsigned int *delta_chain_length,\n \t\t\t       unsigned char *base_sha1)\n {\n-\tstruct packed_git *p = e->p;\n-\tunsigned long offset;\n-\tunsigned char *pack;\n+\tunsigned long val;\n+\tunsigned char *next_sha1;\n \tenum object_type kind;\n \n-\toffset = unpack_object_header(p, e->offset, &kind, size);\n-\tpack = (unsigned char *) p->pack_base + offset;\n-\tif (kind != OBJ_DELTA)\n-\t\t*delta_chain_length = 0;\n-\telse {\n-\t\tunsigned int chain_length = 0;\n-\t\tif (p->pack_size <= offset + 20)\n-\t\t\tdie(\"pack file %s records an incomplete delta base\",\n-\t\t\t    p->pack_name);\n-\t\thashcpy(base_sha1, pack);\n-\t\tdo {\n-\t\t\tstruct pack_entry base_ent;\n-\t\t\tunsigned long junk;\n-\n-\t\t\tfind_pack_entry_one(pack, &base_ent, p);\n-\t\t\toffset = unpack_object_header(p, base_ent.offset,\n-\t\t\t\t\t\t      &kind, &junk);\n-\t\t\tpack = (unsigned char *) p->pack_base + offset;\n-\t\t\tchain_length++;\n-\t\t} while (kind == OBJ_DELTA);\n-\t\t*delta_chain_length = chain_length;\n-\t}\n-\tswitch (kind) {\n-\tcase OBJ_COMMIT:\n-\tcase OBJ_TREE:\n-\tcase OBJ_BLOB:\n-\tcase OBJ_TAG:\n-\t\tstrcpy(type, type_names[kind]);\n-\t\tbreak;\n-\tdefault:\n-\t\tdie(\"corrupted pack file %s containing object of kind %d\",\n-\t\t    p->pack_name, kind);\n+\t*delta_chain_length = 0;\n+\toffset = unpack_object_header(p, offset, &kind, size);\n+\n+\tfor (;;) {\n+\t\tswitch (kind) {\n+\t\tdefault:\n+\t\t\tdie(\"corrupted pack file %s containing object of kind %d\",\n+\t\t    \t\tp->pack_name, kind);\n+\t\tcase OBJ_COMMIT:\n+\t\tcase OBJ_TREE:\n+\t\tcase OBJ_BLOB:\n+\t\tcase OBJ_TAG:\n+\t\t\tstrcpy(type, type_names[kind]);\n+\t\t\t*store_size = 0; /* notyet */\n+\t\t\treturn;\n+\t\tcase OBJ_DELTA:\n+\t\t\tif (p->pack_size <= offset + 20)\n+\t\t\t\tdie(\"pack file %s records an incomplete delta base\",\n+\t\t\t    \t    p->pack_name);\n+\t\t\tnext_sha1 = (unsigned char *) p->pack_base + offset;\n+\t\t\tif (*delta_chain_length == 0)\n+\t\t\t\thashcpy(base_sha1, next_sha1);\n+\t\t\toffset = find_pack_entry_one(next_sha1, p);\n+\t\t\tbreak;\n+\t\t}\n+\t\toffset = unpack_object_header(p, offset, &kind, &val);\n+\t\t(*delta_chain_length)++;\n \t}\n-\t*store_size = 0; /* notyet */\n }\n \n-static int packed_object_info(struct pack_entry *entry,\n+static int packed_object_info(struct packed_git *p, unsigned long offset,\n \t\t\t      char *type, unsigned long *sizep)\n {\n-\tstruct packed_git *p = entry->p;\n-\tunsigned long offset, size, left;\n-\tunsigned char *pack;\n+\tunsigned long size;\n \tenum object_type kind;\n-\tint retval;\n \n-\tif (use_packed_git(p))\n-\t\tdie(\"cannot map packed file\");\n+\toffset = unpack_object_header(p, offset, &kind, &size);\n \n-\toffset = unpack_object_header(p, entry->offset, &kind, &size);\n-\tpack = (unsigned char *) p->pack_base + offset;\n-\tleft = p->pack_size - offset;\n+\tif (kind == OBJ_DELTA)\n+\t\treturn packed_delta_info(p, offset, type, sizep);\n \n \tswitch (kind) {\n-\tcase OBJ_DELTA:\n-\t\tretval = packed_delta_info(pack, size, left, type, sizep, p);\n-\t\tunuse_packed_git(p);\n-\t\treturn retval;\n \tcase OBJ_COMMIT:\n \tcase OBJ_TREE:\n \tcase OBJ_BLOB:\n@@ -1070,7 +1054,6 @@ static int packed_object_info(struct pac\n \t}\n \tif (sizep)\n \t\t*sizep = size;\n-\tunuse_packed_git(p);\n \treturn 0;\n }\n \n@@ -1107,25 +1090,26 @@ static void *unpack_delta_entry(struct p\n \t\t\t\tchar *type,\n \t\t\t\tunsigned long *sizep)\n {\n-\tstruct pack_entry base_ent;\n \tvoid *delta_data, *result, *base;\n-\tunsigned long result_size, base_size;\n-\tunsigned char* base_sha1;\n+\tunsigned long result_size, base_size, base_offset;\n+\tunsigned char *base_sha1;\n \n-\tif ((offset + 20) >= p->pack_size)\n+\tif (p->pack_size < offset + 20)\n \t\tdie(\"truncated pack file\");\n-\n \t/* The base entry _must_ be in the same pack */\n \tbase_sha1 = (unsigned char*)p->pack_base + offset;\n-\tif (!find_pack_entry_one(base_sha1, &base_ent, p))\n+\tbase_offset = find_pack_entry_one(base_sha1, p);\n+\tif (!base_offset)\n \t\tdie(\"failed to find delta-pack base object %s\",\n \t\t    sha1_to_hex(base_sha1));\n-\tbase = unpack_entry_gently(&base_ent, type, &base_size);\n+\toffset += 20;\n+\n+\tbase = unpack_entry_gently(p, base_offset, type, &base_size);\n \tif (!base)\n-\t\tdie(\"failed to read delta-pack base object %s\",\n-\t\t    sha1_to_hex(base_sha1));\n+\t\tdie(\"failed to read delta base object at %lu from %s\",\n+\t\t    base_offset, p->pack_name);\n \n-\tdelta_data = unpack_compressed_entry(p, offset + 20, delta_size);\n+\tdelta_data = unpack_compressed_entry(p, offset, delta_size);\n \tresult = patch_delta(base, base_size,\n \t\t\t     delta_data, delta_size,\n \t\t\t     &result_size);\n@@ -1145,7 +1129,7 @@ static void *unpack_entry(struct pack_en\n \n \tif (use_packed_git(p))\n \t\tdie(\"cannot map packed file\");\n-\tretval = unpack_entry_gently(entry, type, sizep);\n+\tretval = unpack_entry_gently(p, entry->offset, type, sizep);\n \tunuse_packed_git(p);\n \tif (!retval)\n \t\tdie(\"corrupted pack file %s\", p->pack_name);\n@@ -1153,14 +1137,13 @@ static void *unpack_entry(struct pack_en\n }\n \n /* The caller is responsible for use_packed_git()/unuse_packed_git() pair */\n-void *unpack_entry_gently(struct pack_entry *entry,\n+void *unpack_entry_gently(struct packed_git *p, unsigned long offset,\n \t\t\t  char *type, unsigned long *sizep)\n {\n-\tstruct packed_git *p = entry->p;\n-\tunsigned long offset, size;\n+\tunsigned long size;\n \tenum object_type kind;\n \n-\toffset = unpack_object_header(p, entry->offset, &kind, &size);\n+\toffset = unpack_object_header(p, offset, &kind, &size);\n \tswitch (kind) {\n \tcase OBJ_DELTA:\n \t\treturn unpack_delta_entry(p, offset, size, type, sizep);\n@@ -1192,8 +1175,8 @@ int nth_packed_object_sha1(const struct \n \treturn 0;\n }\n \n-int find_pack_entry_one(const unsigned char *sha1,\n-\t\t\tstruct pack_entry *e, struct packed_git *p)\n+unsigned long find_pack_entry_one(const unsigned char *sha1,\n+\t\t\t\t  struct packed_git *p)\n {\n \tunsigned int *level1_ofs = p->index_base;\n \tint hi = ntohl(level1_ofs[*sha1]);\n@@ -1203,12 +1186,8 @@ int find_pack_entry_one(const unsigned c\n \tdo {\n \t\tint mi = (lo + hi) / 2;\n \t\tint cmp = hashcmp((unsigned char *)index + (24 * mi) + 4, sha1);\n-\t\tif (!cmp) {\n-\t\t\te->offset = ntohl(*((unsigned int *) ((char *) index + (24 * mi))));\n-\t\t\thashcpy(e->sha1, sha1);\n-\t\t\te->p = p;\n-\t\t\treturn 1;\n-\t\t}\n+\t\tif (!cmp)\n+\t\t\treturn ntohl(*((unsigned int *) ((char *) index + (24 * mi))));\n \t\tif (cmp > 0)\n \t\t\thi = mi;\n \t\telse\n@@ -1220,6 +1199,8 @@ int find_pack_entry_one(const unsigned c\n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, const char **ignore_packed)\n {\n \tstruct packed_git *p;\n+\tunsigned long offset;\n+\n \tprepare_packed_git();\n \n \tfor (p = packed_git; p; p = p->next) {\n@@ -1231,8 +1212,13 @@ static int find_pack_entry(const unsigne\n \t\t\tif (*ig)\n \t\t\t\tcontinue;\n \t\t}\n-\t\tif (find_pack_entry_one(sha1, e, p))\n+\t\toffset = find_pack_entry_one(sha1, p);\n+\t\tif (offset) {\n+\t\t\te->offset = offset;\n+\t\t\te->p = p;\n+\t\t\thashcpy(e->sha1, sha1);\n \t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -1241,10 +1227,9 @@ struct packed_git *find_sha1_pack(const \n \t\t\t\t  struct packed_git *packs)\n {\n \tstruct packed_git *p;\n-\tstruct pack_entry e;\n \n \tfor (p = packs; p; p = p->next) {\n-\t\tif (find_pack_entry_one(sha1, &e, p))\n+\t\tif (find_pack_entry_one(sha1, p))\n \t\t\treturn p;\n \t}\n \treturn NULL;\n@@ -1263,12 +1248,16 @@ int sha1_object_info(const unsigned char\n \tif (!map) {\n \t\tstruct pack_entry e;\n \n-\t\tif (find_pack_entry(sha1, &e, NULL))\n-\t\t\treturn packed_object_info(&e, type, sizep);\n-\t\treprepare_packed_git();\n-\t\tif (find_pack_entry(sha1, &e, NULL))\n-\t\t\treturn packed_object_info(&e, type, sizep);\n-\t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n+\t\tif (!find_pack_entry(sha1, &e, NULL)) {\n+\t\t\treprepare_packed_git();\n+\t\t\tif (!find_pack_entry(sha1, &e, NULL))\n+\t\t\t\treturn error(\"unable to find %s\", sha1_to_hex(sha1));\n+\t\t}\n+\t\tif (use_packed_git(e.p))\n+\t\t\tdie(\"cannot map packed file\");\n+\t\tstatus = packed_object_info(e.p, e.offset, type, sizep);\n+\t\tunuse_packed_git(e.p);\n+\t\treturn status;\n \t}\n \tif (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)\n \t\tstatus = error(\"unable to unpack %s header\",\n"},{"id":"27454","messageId":"7v7izvug3b.fsf@assigned-by-dhcp.cox.net","threadId":"5630","inReplyTo":"Pine.LNX.4.64.0609210004550.2627@xanadu.home","subject":"Re: [PATCH 2/6] many cleanups to sha1_file.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-09-23T08:44:56Z","receivedAt":"2006-09-23T08:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> Those cleanups are mainly to set the table for the support of deltas\n> with base objects referenced by offsets instead of sha1.  This means\n> that many pack lookup functions are converted to take a pack/offset\n> tuple instead of a sha1.\n\nThere still remains some \"struct pack_entry\" and I wonder if it\nwould make sense to get rid of them if only for consistency's\nsake.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index b89edb9..6fae766 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -926,8 +925,8 @@ static int packed_delta_info(unsigned ch\n>  \n>  \t\tmemset(&stream, 0, sizeof(stream));\n>  \n> -\t\tdata = stream.next_in = base_sha1 + 20;\n> -\t\tstream.avail_in = left - 20;\n> +\t\tdata = stream.next_in = (unsigned char *) p->pack_base + offset;;\n> +\t\tstream.avail_in = p->pack_size - offset;\n>  \t\tstream.next_out = delta_head;\n>  \t\tstream.avail_out = sizeof(delta_head);\n>  \n\n\";;\"?\n\n> @@ -989,75 +988,60 @@ int check_reuse_pack_delta(struct packed\n>  ...\n> +static int packed_object_info(struct packed_git *p, unsigned long offset,\n>  \t\t\t      char *type, unsigned long *sizep)\n>  {\n> +\tunsigned long size;\n>  \tenum object_type kind;\n>  \n> -\tif (use_packed_git(p))\n> -\t\tdie(\"cannot map packed file\");\n> +\toffset = unpack_object_header(p, offset, &kind, &size);\n>  \n\nDo all callers of packed_object_info() call use_packed_git to\nmake sure p is mapped?  Ah sha1_object_info() is the only one\nexcept packed_delta_info() that calls itself and it protects it\nwith use_packed_git(), so you are safe.\n\nWe would need to revamp use/unuse code when we implement the\npartial mapping anyway, but we still need to get this right for\nnow.\n"},{"id":"27530","messageId":"Pine.LNX.4.64.0609240006001.9349@xanadu.home","threadId":"5630","inReplyTo":"7v7izvug3b.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/6] many cleanups to sha1_file.c","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-09-24T04:17:16Z","receivedAt":"2006-09-24T04:17:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 23 Sep 2006, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > Those cleanups are mainly to set the table for the support of deltas\n> > with base objects referenced by offsets instead of sha1.  This means\n> > that many pack lookup functions are converted to take a pack/offset\n> > tuple instead of a sha1.\n> \n> There still remains some \"struct pack_entry\" and I wonder if it\n> would make sense to get rid of them if only for consistency's\n> sake.\n\nMaybe.  But since those are the minimum needed for base offset support \nI'd prefer if the extra cleanup is made separate not to mix too many \nissues at once.\n\n> > diff --git a/sha1_file.c b/sha1_file.c\n> > index b89edb9..6fae766 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -926,8 +925,8 @@ static int packed_delta_info(unsigned ch\n> >  \n> >  \t\tmemset(&stream, 0, sizeof(stream));\n> >  \n> > -\t\tdata = stream.next_in = base_sha1 + 20;\n> > -\t\tstream.avail_in = left - 20;\n> > +\t\tdata = stream.next_in = (unsigned char *) p->pack_base + offset;;\n> > +\t\tstream.avail_in = p->pack_size - offset;\n> >  \t\tstream.next_out = delta_head;\n> >  \t\tstream.avail_out = sizeof(delta_head);\n> >  \n> \n> \";;\"?\n\nTypo.\n\n> > @@ -989,75 +988,60 @@ int check_reuse_pack_delta(struct packed\n> >  ...\n> > +static int packed_object_info(struct packed_git *p, unsigned long offset,\n> >  \t\t\t      char *type, unsigned long *sizep)\n> >  {\n> > +\tunsigned long size;\n> >  \tenum object_type kind;\n> >  \n> > -\tif (use_packed_git(p))\n> > -\t\tdie(\"cannot map packed file\");\n> > +\toffset = unpack_object_header(p, offset, &kind, &size);\n> >  \n> \n> Do all callers of packed_object_info() call use_packed_git to\n> make sure p is mapped?\n\nEasy since the function is static.\n\n> Ah sha1_object_info() is the only one\n> except packed_delta_info() that calls itself and it protects it\n> with use_packed_git(), so you are safe.\n\nRight.  The only entry point to those functions is sha1_object_info() \nwhere I moved the use/unuse and all the other call instances are due to \nrecursion. >  It seemed to me a bit wasteful to call use() so many times \nto unuse() the same amount afterwards while only once is needed.\n\n> We would need to revamp use/unuse code when we implement the\n> partial mapping anyway, but we still need to get this right for\n> now.\n\nProbably moving them near the actual pack content access.\n\nMay I assume you'll take care of the extra ; typo?\n\n\nNicolas\n"}]}