{"thread":{"id":"35853","subject":"[PATCH v2] Make the global packed_git variable static to sha1_file.c.","startedAt":"2014-02-12T18:52:06Z","lastAt":"2014-02-13T23:15:06Z","messageCount":4,"participants":["szager@chromium.org","Junio C Hamano","Stefan Zager"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"234678","messageId":"52fbc2d6.7kJdh2GOdWTzNcPK%szager@chromium.org","threadId":"35853","inReplyTo":null,"subject":"[PATCH v2] Make the global packed_git variable static to sha1_file.c.","fromName":"","fromEmail":"szager@chromium.org","sentAt":"2014-02-12T18:52:06Z","receivedAt":"2014-02-12T18:52:06Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":">From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001\nFrom: Stefan Zager <szager@chromium.org>\nDate: Mon, 10 Feb 2014 16:55:12 -0800\nSubject: [PATCH] Make the global packed_git variable static to sha1_file.c.\n\nThis is a first step in making the codebase thread-safe.  By and\nlarge, the operations which might benefit from threading are those\nthat work with pack files (e.g., checkout, blame), so the focus of\nthis patch is stop leaking the global list of pack files outside of\nsha1_file.c.\n\nThe next step will be to control access to the list of pack files\nwith a mutex.  However, that alone is not enough to make pack file\naccess thread safe.  Even in a read-only operation, the window list\nassociated with each pack file will need to be controlled.\nAdditionally, the global counters in sha1_file.c will need to be\ncontrolled.\n\nThis patch is a pure refactor with no functional changes, so it\nshouldn't require any additional tests.  Adding the actual locks\nwill be a functional change, and will require additional tests.\n\nSigned-off-by: Stefan Zager <szager@chromium.org>\n---\n builtin/count-objects.c  |  44 ++++++-----\n builtin/fsck.c           |  46 +++++++-----\n builtin/gc.c             |  26 +++----\n builtin/pack-objects.c   | 188 ++++++++++++++++++++++++++++-------------------\n builtin/pack-redundant.c |  37 +++++++---\n cache.h                  |  16 +++-\n fast-import.c            |   4 +-\n http-backend.c           |  28 ++++---\n http-push.c              |   4 +-\n http-walker.c            |   2 +-\n pack-revindex.c          |  20 ++---\n server-info.c            |  35 +++++----\n sha1_file.c              |  35 ++++++++-\n sha1_name.c              |  18 ++++-\n 14 files changed, 315 insertions(+), 188 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex a7f70cb..6554dfe 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -83,14 +83,32 @@ static char const * const count_objects_usage[] = {\n \tNULL\n };\n \n+struct pack_data {\n+\tunsigned long packed;\n+\toff_t size_pack;\n+\tunsigned long num_pack;\n+};\n+\n+int pack_data_fn(struct packed_git *p, void *data)\n+{\n+\tstruct pack_data *pd = (struct pack_data *) data;\n+\tif (p->pack_local && !open_pack_index(p)) {\n+\t\tpd->packed += p->num_objects;\n+\t\tpd->size_pack += p->pack_size + p->index_size;\n+\t\tpd->num_pack++;\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_count_objects(int argc, const char **argv, const char *prefix)\n {\n \tint i, verbose = 0, human_readable = 0;\n \tconst char *objdir = get_object_directory();\n \tint len = strlen(objdir);\n \tchar *path = xmalloc(len + 50);\n-\tunsigned long loose = 0, packed = 0, packed_loose = 0;\n+\tunsigned long loose = 0, packed_loose = 0;\n \toff_t loose_size = 0;\n+\tstruct pack_data pd = {0,0,0};\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_BOOL('H', \"human-readable\", &human_readable,\n@@ -118,41 +136,29 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tclosedir(d);\n \t}\n \tif (verbose) {\n-\t\tstruct packed_git *p;\n-\t\tunsigned long num_pack = 0;\n-\t\toff_t size_pack = 0;\n \t\tstruct strbuf loose_buf = STRBUF_INIT;\n \t\tstruct strbuf pack_buf = STRBUF_INIT;\n \t\tstruct strbuf garbage_buf = STRBUF_INIT;\n-\t\tif (!packed_git)\n-\t\t\tprepare_packed_git();\n-\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\tif (!p->pack_local)\n-\t\t\t\tcontinue;\n-\t\t\tif (open_pack_index(p))\n-\t\t\t\tcontinue;\n-\t\t\tpacked += p->num_objects;\n-\t\t\tsize_pack += p->pack_size + p->index_size;\n-\t\t\tnum_pack++;\n-\t\t}\n+\t\tprepare_packed_git();\n+\t\tforeach_packed_git(pack_data_fn, NULL, &pd);\n \n \t\tif (human_readable) {\n \t\t\tstrbuf_humanise_bytes(&loose_buf, loose_size);\n-\t\t\tstrbuf_humanise_bytes(&pack_buf, size_pack);\n+\t\t\tstrbuf_humanise_bytes(&pack_buf, pd.size_pack);\n \t\t\tstrbuf_humanise_bytes(&garbage_buf, size_garbage);\n \t\t} else {\n \t\t\tstrbuf_addf(&loose_buf, \"%lu\",\n \t\t\t\t    (unsigned long)(loose_size / 1024));\n \t\t\tstrbuf_addf(&pack_buf, \"%lu\",\n-\t\t\t\t    (unsigned long)(size_pack / 1024));\n+\t\t\t\t    (unsigned long)(pd.size_pack / 1024));\n \t\t\tstrbuf_addf(&garbage_buf, \"%lu\",\n \t\t\t\t    (unsigned long)(size_garbage / 1024));\n \t\t}\n \n \t\tprintf(\"count: %lu\\n\", loose);\n \t\tprintf(\"size: %s\\n\", loose_buf.buf);\n-\t\tprintf(\"in-pack: %lu\\n\", packed);\n-\t\tprintf(\"packs: %lu\\n\", num_pack);\n+\t\tprintf(\"in-pack: %lu\\n\", pd.packed);\n+\t\tprintf(\"packs: %lu\\n\", pd.num_pack);\n \t\tprintf(\"size-pack: %s\\n\", pack_buf.buf);\n \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n \t\tprintf(\"garbage: %lu\\n\", garbage);\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 1affdd5..5f31466 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -624,6 +624,29 @@ static struct option fsck_opts[] = {\n \tOPT_END(),\n };\n \n+struct verify_packs_data {\n+\tstruct progress *progress;\n+\tuint32_t total;\n+\tuint32_t count;\n+};\n+\n+static int count_packs_fn(struct packed_git *p, void *data)\n+{\n+\tstruct verify_packs_data *vpd = (struct verify_packs_data *) data;\n+\tif (!open_pack_index(p))\n+\t\tvpd->total++;\n+\treturn 0;\n+}\n+\n+static int verify_packs_fn(struct packed_git *p, void *data)\n+{\n+\tstruct verify_packs_data *vpd = (struct verify_packs_data *) data;\n+\tif (verify_pack(p, fsck_obj_buffer, vpd->progress, vpd->count))\n+\t\terrors_found |= ERROR_PACK;\n+\tvpd->count += p->num_objects;\n+\treturn 0;\n+}\n+\n int cmd_fsck(int argc, const char **argv, const char *prefix)\n {\n \tint i, heads;\n@@ -657,29 +680,16 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (check_full) {\n-\t\tstruct packed_git *p;\n-\t\tuint32_t total = 0, count = 0;\n-\t\tstruct progress *progress = NULL;\n+\t\tstruct verify_packs_data vpd = {0,0,0};\n \n \t\tprepare_packed_git();\n \n \t\tif (show_progress) {\n-\t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (open_pack_index(p))\n-\t\t\t\t\tcontinue;\n-\t\t\t\ttotal += p->num_objects;\n-\t\t\t}\n-\n-\t\t\tprogress = start_progress(\"Checking objects\", total);\n-\t\t}\n-\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t/* verify gives error messages itself */\n-\t\t\tif (verify_pack(p, fsck_obj_buffer,\n-\t\t\t\t\tprogress, count))\n-\t\t\t\terrors_found |= ERROR_PACK;\n-\t\t\tcount += p->num_objects;\n+\t\t\tforeach_packed_git(count_packs_fn, NULL, &vpd);\n+\t\t\tvpd.progress = start_progress(\"Checking objects\", vpd.total);\n \t\t}\n-\t\tstop_progress(&progress);\n+\t\tforeach_packed_git(verify_packs_fn, NULL, &vpd);\n+\t\tstop_progress(&vpd.progress);\n \t}\n \n \theads = 0;\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c19545d..7fc49bb 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -125,26 +125,26 @@ static int too_many_loose_objects(void)\n \treturn needed;\n }\n \n+static int count_packs_fn(struct packed_git *p, void *data)\n+{\n+\t/*\n+\t * Perhaps check the size of the pack and count only\n+\t * very small ones here?\n+\t */\n+\tif (p->pack_local && !p->pack_keep)\n+\t\t(*((int *) data))++;\n+\treturn 0;\n+}\n+\n static int too_many_packs(void)\n {\n-\tstruct packed_git *p;\n-\tint cnt;\n+\tint cnt = 0;\n \n \tif (gc_auto_pack_limit <= 0)\n \t\treturn 0;\n \n \tprepare_packed_git();\n-\tfor (cnt = 0, p = packed_git; p; p = p->next) {\n-\t\tif (!p->pack_local)\n-\t\t\tcontinue;\n-\t\tif (p->pack_keep)\n-\t\t\tcontinue;\n-\t\t/*\n-\t\t * Perhaps check the size of the pack and count only\n-\t\t * very small ones here?\n-\t\t */\n-\t\tcnt++;\n-\t}\n+\tforeach_packed_git(count_packs_fn, NULL, &cnt);\n \treturn gc_auto_pack_limit <= cnt;\n }\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 541667f..bc3074b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -900,14 +900,45 @@ static int no_try_delta(const char *path)\n \treturn 0;\n }\n \n+struct find_pack_data {\n+\tconst unsigned char *sha1;\n+\toff_t offset;\n+\tstruct packed_git *found_pack;\n+\tint exclude;\n+\tint found_non_local_pack;\n+\tint found_pack_keep;\n+};\n+\n+static int find_pack_fn(struct packed_git *p, void *data)\n+{\n+\tstruct find_pack_data *fpd = (struct find_pack_data *) data;\n+\toff_t offset = find_pack_entry_one(fpd->sha1, p);\n+\tif (offset) {\n+\t\tif (!fpd->found_pack) {\n+\t\t\tif (!is_pack_valid(p)) {\n+\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tfpd->offset = offset;\n+\t\t\tfpd->found_pack = p;\n+\t\t}\n+\t\tif (fpd->exclude)\n+\t\t\treturn 1;\n+\t\tif (!p->pack_local)\n+\t\t\tfpd->found_non_local_pack = 1;\n+\t\telse if (p->pack_keep)\n+\t\t\tfpd->found_pack_keep = 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\t\t    const char *name, int exclude)\n {\n \tstruct object_entry *entry;\n-\tstruct packed_git *p, *found_pack = NULL;\n-\toff_t found_offset = 0;\n \tint ix;\n \tuint32_t hash = name_hash(name);\n+\tstruct find_pack_data fpd;\n \n \tix = nr_objects ? locate_object_entry_hash(sha1) : -1;\n \tif (ix >= 0) {\n@@ -923,27 +954,18 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \tif (!exclude && local && has_loose_object_nonlocal(sha1))\n \t\treturn 0;\n \n-\tfor (p = packed_git; p; p = p->next) {\n-\t\toff_t offset = find_pack_entry_one(sha1, p);\n-\t\tif (offset) {\n-\t\t\tif (!found_pack) {\n-\t\t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t\tfound_offset = offset;\n-\t\t\t\tfound_pack = p;\n-\t\t\t}\n-\t\t\tif (exclude)\n-\t\t\t\tbreak;\n-\t\t\tif (incremental)\n-\t\t\t\treturn 0;\n-\t\t\tif (local && !p->pack_local)\n-\t\t\t\treturn 0;\n-\t\t\tif (ignore_packed_keep && p->pack_local && p->pack_keep)\n-\t\t\t\treturn 0;\n-\t\t}\n-\t}\n+\tfpd.sha1 = sha1;\n+\tfpd.offset = 0;\n+\tfpd.found_pack = NULL;\n+\tfpd.exclude = exclude;\n+\tfpd.found_non_local_pack = fpd.found_pack_keep = 0;\n+\n+\tforeach_packed_git(find_pack_fn, NULL, &fpd);\n+\tif (fpd.found_pack && !exclude &&\n+\t    (incremental ||\n+\t     (local && fpd.found_non_local_pack) ||\n+\t     (ignore_packed_keep && fpd.found_pack_keep)))\n+\t\treturn 0;\n \n \tif (nr_objects >= nr_alloc) {\n \t\tnr_alloc = (nr_alloc  + 1024) * 3 / 2;\n@@ -960,9 +982,9 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\tentry->preferred_base = 1;\n \telse\n \t\tnr_result++;\n-\tif (found_pack) {\n-\t\tentry->in_pack = found_pack;\n-\t\tentry->in_pack_offset = found_offset;\n+\tif (fpd.found_pack) {\n+\t\tentry->in_pack = fpd.found_pack;\n+\t\tentry->in_pack_offset = fpd.offset;\n \t}\n \n \tif (object_ix_hashsz * 3 <= nr_objects * 4)\n@@ -2257,35 +2279,40 @@ static int ofscmp(const void *a_, const void *b_)\n \t\treturn hashcmp(a->object->sha1, b->object->sha1);\n }\n \n-static void add_objects_in_unpacked_packs(struct rev_info *revs)\n+static int add_objects_fn(struct packed_git *p, void *data)\n {\n-\tstruct packed_git *p;\n-\tstruct in_pack in_pack;\n+\tstruct in_pack *ip = (struct in_pack *) data;\n \tuint32_t i;\n+\tconst unsigned char *sha1;\n+\tstruct object *o;\n \n-\tmemset(&in_pack, 0, sizeof(in_pack));\n-\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tconst unsigned char *sha1;\n-\t\tstruct object *o;\n-\n-\t\tif (!p->pack_local || p->pack_keep)\n-\t\t\tcontinue;\n+\tif (p->pack_local && !p->pack_keep) {\n \t\tif (open_pack_index(p))\n \t\t\tdie(\"cannot open pack index\");\n \n-\t\tALLOC_GROW(in_pack.array,\n-\t\t\t   in_pack.nr + p->num_objects,\n-\t\t\t   in_pack.alloc);\n+\t\tALLOC_GROW(ip->array,\n+\t\t\t   ip->nr + p->num_objects,\n+\t\t\t   ip->alloc);\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tsha1 = nth_packed_object_sha1(p, i);\n \t\t\to = lookup_unknown_object(sha1);\n \t\t\tif (!(o->flags & OBJECT_ADDED))\n-\t\t\t\tmark_in_pack_object(o, p, &in_pack);\n+\t\t\t\tmark_in_pack_object(o, p, ip);\n \t\t\to->flags |= OBJECT_ADDED;\n \t\t}\n \t}\n+\treturn 0;\n+}\n+\n+static void add_objects_in_unpacked_packs(struct rev_info *revs)\n+{\n+\tstruct in_pack in_pack;\n+\tuint32_t i;\n+\n+\tmemset(&in_pack, 0, sizeof(in_pack));\n+\n+\tforeach_packed_git(add_objects_fn, NULL, &in_pack);\n \n \tif (in_pack.nr) {\n \t\tqsort(in_pack.array, in_pack.nr, sizeof(in_pack.array[0]),\n@@ -2298,54 +2325,63 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)\n \tfree(in_pack.array);\n }\n \n+struct nonlocal_or_keep_data {\n+\tstruct packed_git **last_found;\n+\tconst unsigned char *sha1;\n+\tint found;\n+};\n+\n+static int find_nonlocal_or_keep_fn(struct packed_git *p, void *data)\n+{\n+\tstruct nonlocal_or_keep_data *d = (struct nonlocal_or_keep_data *) data;\n+\tif ((!p->pack_local || p->pack_keep) &&\n+\t    find_pack_entry_one(d->sha1, p)) {\n+\t\t*d->last_found = p;\n+\t\td->found = 1;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n {\n \tstatic struct packed_git *last_found = (void *)1;\n-\tstruct packed_git *p;\n+\tstruct nonlocal_or_keep_data d;\n+\tstruct packed_git *hint;\n \n-\tp = (last_found != (void *)1) ? last_found : packed_git;\n+\td.last_found = &last_found;\n+\td.sha1 = sha1;\n+\td.found = 0;\n \n-\twhile (p) {\n-\t\tif ((!p->pack_local || p->pack_keep) &&\n-\t\t\tfind_pack_entry_one(sha1, p)) {\n-\t\t\tlast_found = p;\n-\t\t\treturn 1;\n-\t\t}\n-\t\tif (p == last_found)\n-\t\t\tp = packed_git;\n-\t\telse\n-\t\t\tp = p->next;\n-\t\tif (p == last_found)\n-\t\t\tp = p->next;\n-\t}\n-\treturn 0;\n+\thint = (last_found != (void *)1) ? last_found : NULL;\n+\n+\tforeach_packed_git(find_nonlocal_or_keep_fn, hint, &d);\n+\treturn d.found;\n }\n \n-static void loosen_unused_packed_objects(struct rev_info *revs)\n+static int loosen_unused_objects_fn(struct packed_git *p, void *data)\n {\n-\tstruct packed_git *p;\n+\tconst unsigned char *sha1 = (const unsigned char *) data;\n \tuint32_t i;\n-\tconst unsigned char *sha1;\n \n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tif (!p->pack_local || p->pack_keep)\n-\t\t\tcontinue;\n+\tif (!p->pack_local || p->pack_keep)\n+\t\treturn 0;\n \n-\t\tif (unpack_unreachable_expiration &&\n-\t\t    p->mtime < unpack_unreachable_expiration)\n-\t\t\tcontinue;\n+\tif (unpack_unreachable_expiration &&\n+\t\tp->mtime < unpack_unreachable_expiration)\n+\t\treturn 0;\n \n-\t\tif (open_pack_index(p))\n-\t\t\tdie(\"cannot open pack index\");\n+\tif (open_pack_index(p))\n+\t\tdie(\"cannot open pack index\");\n \n-\t\tfor (i = 0; i < p->num_objects; i++) {\n-\t\t\tsha1 = nth_packed_object_sha1(p, i);\n-\t\t\tif (!locate_object_entry(sha1) &&\n-\t\t\t\t!has_sha1_pack_kept_or_nonlocal(sha1))\n-\t\t\t\tif (force_object_loose(sha1, p->mtime))\n-\t\t\t\t\tdie(\"unable to force loose object\");\n-\t\t}\n+\tfor (i = 0; i < p->num_objects; i++) {\n+\t\tsha1 = nth_packed_object_sha1(p, i);\n+\t\tif (!locate_object_entry(sha1) &&\n+\t\t\t!has_sha1_pack_kept_or_nonlocal(sha1))\n+\t\t\tif (force_object_loose(sha1, p->mtime))\n+\t\t\t\tdie(\"unable to force loose object\");\n \t}\n+\treturn 0;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -2383,7 +2419,7 @@ static void get_object_list(int ac, const char **av)\n \tif (keep_unreachable)\n \t\tadd_objects_in_unpacked_packs(&revs);\n \tif (unpack_unreachable)\n-\t\tloosen_unused_packed_objects(&revs);\n+\t\tforeach_packed_git(loosen_unused_objects_fn, NULL, NULL);\n }\n \n static int option_parse_index_version(const struct option *opt,\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 649c3aa..cc44502 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -567,29 +567,42 @@ static struct pack_list * add_pack(struct packed_git *p)\n \t\treturn pack_list_insert(&altodb_packs, &l);\n }\n \n+struct add_pack_data {\n+\tconst char *filename;\n+\tint found;\n+\tstruct pack_list *added_pack;\n+};\n+\n+static int add_pack_fn(struct packed_git *p, void *data)\n+{\n+\tstruct add_pack_data *apd = (struct add_pack_data *) data;\n+\tif (apd->filename && strstr(p->pack_name, apd->filename)) {\n+\t\tapd->found = 1;\n+\t\tapd->added_pack = add_pack(p);\n+\t\treturn 1;\n+\t} else if (!apd->filename) {\n+\t\tadd_pack(p);\n+\t}\n+\treturn 0;\n+}\n+\n static struct pack_list * add_pack_file(const char *filename)\n {\n-\tstruct packed_git *p = packed_git;\n+\tstruct add_pack_data apd = {filename,0,NULL};\n \n \tif (strlen(filename) < 40)\n \t\tdie(\"Bad pack filename: %s\", filename);\n \n-\twhile (p) {\n-\t\tif (strstr(p->pack_name, filename))\n-\t\t\treturn add_pack(p);\n-\t\tp = p->next;\n-\t}\n+\tforeach_packed_git(add_pack_fn, NULL, &apd);\n+\tif (apd.found)\n+\t\treturn apd.added_pack;\n \tdie(\"Filename %s not found in packed_git\", filename);\n }\n \n static void load_all(void)\n {\n-\tstruct packed_git *p = packed_git;\n-\n-\twhile (p) {\n-\t\tadd_pack(p);\n-\t\tp = p->next;\n-\t}\n+\tstruct add_pack_data apd = {NULL,0,NULL};\n+\tforeach_packed_git(add_pack_fn, NULL, &apd);\n }\n \n int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\ndiff --git a/cache.h b/cache.h\nindex dc040fb..542a9d9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1044,7 +1044,7 @@ struct pack_window {\n \tunsigned int inuse_cnt;\n };\n \n-extern struct packed_git {\n+struct packed_git {\n \tstruct packed_git *next;\n \tstruct pack_window *windows;\n \toff_t pack_size;\n@@ -1062,7 +1062,7 @@ extern struct packed_git {\n \tunsigned char sha1[20];\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n \tchar pack_name[FLEX_ARRAY]; /* more */\n-} *packed_git;\n+};\n \n struct pack_entry {\n \toff_t offset;\n@@ -1070,6 +1070,18 @@ struct pack_entry {\n \tstruct packed_git *p;\n };\n \n+/* packed_git_foreach_fn implementations should return zero\n+ * to continue the traversal, non-zero to halt.\n+ */\n+typedef int (*packed_git_foreach_fn)(struct packed_git *, void *);\n+\n+/* The 'hint' argument is for the commonly-used 'last found pack' optimization.\n+ * It can be NULL.\n+ */\n+extern void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint, void *data);\n+\n+extern size_t packed_git_count();\n+extern size_t packed_git_local_count();\n extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);\n \n /* A hook for count-objects to report invalid files in pack directory */\ndiff --git a/fast-import.c b/fast-import.c\nindex 4fd18a3..11d1f22 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1037,7 +1037,7 @@ static int store_object(\n \tif (e->idx.offset) {\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n-\t} else if (find_sha1_pack(sha1, packed_git)) {\n+\t} else if (find_sha1_pack(sha1, NULL)) {\n \t\te->type = type;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\n@@ -1238,7 +1238,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)\n \t\tduplicate_count_by_type[OBJ_BLOB]++;\n \t\ttruncate_pack(&checkpoint);\n \n-\t} else if (find_sha1_pack(sha1, packed_git)) {\n+\t} else if (find_sha1_pack(sha1, NULL)) {\n \t\te->type = OBJ_BLOB;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..cf1f2a4 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -438,25 +438,29 @@ static void get_head(char *arg)\n \tstrbuf_release(&buf);\n }\n \n+struct pack_info_data {\n+\tstruct strbuf *buf;\n+\tsize_t objdirlen;\n+};\n+\n+static int append_pack_info_fn(struct packed_git *p, void *data)\n+{\n+\tstruct pack_info_data *d = (struct pack_info_data *) data;\n+\tif (p->pack_local)\n+\t\tstrbuf_addf(d->buf, \"P %s\\n\", p->pack_name + d->objdirlen + 6);\n+\treturn 0;\n+}\n+\n static void get_info_packs(char *arg)\n {\n \tsize_t objdirlen = strlen(get_object_directory());\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct packed_git *p;\n-\tsize_t cnt = 0;\n+\tstruct pack_info_data d = {&buf, objdirlen};\n \n \tselect_getanyfile();\n \tprepare_packed_git();\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tif (p->pack_local)\n-\t\t\tcnt++;\n-\t}\n-\n-\tstrbuf_grow(&buf, cnt * 53 + 2);\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tif (p->pack_local)\n-\t\t\tstrbuf_addf(&buf, \"P %s\\n\", p->pack_name + objdirlen + 6);\n-\t}\n+\tstrbuf_grow(&buf, packed_git_local_count() * 53 + 2);\n+\tforeach_packed_git(append_pack_info_fn, NULL, &d);\n \tstrbuf_addch(&buf, '\\n');\n \n \thdr_nocache();\ndiff --git a/http-push.c b/http-push.c\nindex d4b40c9..f2c4256 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -304,7 +304,7 @@ static void start_fetch_packed(struct transfer_request *request)\n \tstruct transfer_request *check_request = request_queue_head;\n \tstruct http_pack_request *preq;\n \n-\ttarget = find_sha1_pack(request->obj->sha1, repo->packs);\n+\ttarget = repo->packs ? find_sha1_pack(request->obj->sha1, repo->packs) : NULL;\n \tif (!target) {\n \t\tfprintf(stderr, \"Unable to fetch %s, will not be able to update server info refs\\n\", sha1_to_hex(request->obj->sha1));\n \t\trepo->can_update_info_refs = 0;\n@@ -677,7 +677,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)\n \t\tget_remote_object_list(obj->sha1[0]);\n \tif (obj->flags & (REMOTE | PUSHING))\n \t\treturn 0;\n-\ttarget = find_sha1_pack(obj->sha1, repo->packs);\n+\ttarget = repo->packs ? find_sha1_pack(obj->sha1, repo->packs) : NULL;\n \tif (target) {\n \t\tobj->flags |= REMOTE;\n \t\treturn 0;\ndiff --git a/http-walker.c b/http-walker.c\nindex 1516c5e..d9d758a 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -405,7 +405,7 @@ static int http_fetch_pack(struct walker *walker, struct alt_base *repo, unsigne\n \n \tif (fetch_indices(walker, repo))\n \t\treturn -1;\n-\ttarget = find_sha1_pack(sha1, repo->packs);\n+\ttarget = repo->packs ? find_sha1_pack(sha1, repo->packs) : NULL;\n \tif (!target)\n \t\treturn -1;\n \ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex b4d2b35..f4b6b7f 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -40,23 +40,25 @@ static int pack_revindex_ix(struct packed_git *p)\n \treturn -1 - i;\n }\n \n+static int init_pack_revindex_fn(struct packed_git *p, void *data)\n+{\n+\t/* revindex elements are lazily initialized */\n+\tint num = pack_revindex_ix(p);\n+\tnum = - 1 - num;\n+\tpack_revindex[num].p = p;\n+\treturn 0;\n+}\n+\n static void init_pack_revindex(void)\n {\n \tint num;\n-\tstruct packed_git *p;\n \n-\tfor (num = 0, p = packed_git; p; p = p->next)\n-\t\tnum++;\n+\tnum = packed_git_count();\n \tif (!num)\n \t\treturn;\n \tpack_revindex_hashsz = num * 11;\n \tpack_revindex = xcalloc(sizeof(*pack_revindex), pack_revindex_hashsz);\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\tnum = pack_revindex_ix(p);\n-\t\tnum = - 1 - num;\n-\t\tpack_revindex[num].p = p;\n-\t}\n-\t/* revindex elements are lazily initialized */\n+\tforeach_packed_git(init_pack_revindex_fn, NULL, NULL);\n }\n \n /*\ndiff --git a/server-info.c b/server-info.c\nindex 9ec744e..70e5a26 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -151,9 +151,20 @@ static int compare_info(const void *a_, const void *b_)\n \t\treturn 1;\n }\n \n+static int init_pack_info_fn(struct packed_git *p, void *data)\n+{\n+\tint *idx = (int *) data;\n+\tif (p->pack_local) {\n+\t\tinfo[*idx] = xcalloc(1, sizeof(struct pack_info));\n+\t\tinfo[*idx]->p = p;\n+\t\tinfo[*idx]->old_num = -1;\n+\t\t(*idx)++;\n+\t}\n+\treturn 0;\n+}\n+\n static void init_pack_info(const char *infofile, int force)\n {\n-\tstruct packed_git *p;\n \tint stale;\n \tint i = 0;\n \n@@ -161,24 +172,12 @@ static void init_pack_info(const char *infofile, int force)\n \tobjdirlen = strlen(objdir);\n \n \tprepare_packed_git();\n-\tfor (p = packed_git; p; p = p->next) {\n-\t\t/* we ignore things on alternate path since they are\n-\t\t * not available to the pullers in general.\n-\t\t */\n-\t\tif (!p->pack_local)\n-\t\t\tcontinue;\n-\t\ti++;\n-\t}\n-\tnum_pack = i;\n+\t/* we ignore things on alternate path since they are\n+\t * not available to the pullers in general.\n+\t */\n+\tnum_pack = packed_git_local_count();\n \tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n-\tfor (i = 0, p = packed_git; p; p = p->next) {\n-\t\tif (!p->pack_local)\n-\t\t\tcontinue;\n-\t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n-\t\tinfo[i]->p = p;\n-\t\tinfo[i]->old_num = -1;\n-\t\ti++;\n-\t}\n+\tforeach_packed_git(init_pack_info_fn, NULL, &i);\n \n \tif (infofile && !force)\n \t\tstale = read_pack_info_file(infofile);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6e8c05d..aeeb7e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -60,6 +60,7 @@ static struct cached_object empty_tree = {\n \t0\n };\n \n+static struct packed_git *packed_git;\n static struct packed_git *last_found_pack;\n \n static struct cached_object *find_cached_object(const unsigned char *sha1)\n@@ -468,7 +469,6 @@ static unsigned int pack_open_fds;\n static unsigned int pack_max_fds;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n-struct packed_git *packed_git;\n \n void pack_report(void)\n {\n@@ -1091,6 +1091,37 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n \treturn p;\n }\n \n+void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint, void *data)\n+{\n+\tstruct packed_git *p;\n+\tif (hint && ((*fn)(hint, data)))\n+\t\treturn;\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tif (p != hint && (*fn)(p, data))\n+\t\t\treturn;\n+}\n+\n+size_t packed_git_count()\n+{\n+\tsize_t res = 0;\n+\tstruct packed_git *p;\n+\n+\tfor (p = packed_git; p; p = p->next)\n+\t\t++res;\n+\treturn res;\n+}\n+\n+size_t packed_git_local_count()\n+{\n+\tsize_t res = 0;\n+\tstruct packed_git *p;\n+\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tif (p->pack_local)\n+\t\t\t++res;\n+\treturn res;\n+}\n+\n struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)\n {\n \tconst char *path = sha1_pack_name(sha1);\n@@ -2474,6 +2505,8 @@ struct packed_git *find_sha1_pack(const unsigned char *sha1,\n {\n \tstruct packed_git *p;\n \n+\tif (!packs)\n+\t\tpacks = packed_git;\n \tfor (p = packs; p; p = p->next) {\n \t\tif (find_pack_entry_one(sha1, p))\n \t\t\treturn p;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6fca869..2404cda 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -176,14 +176,26 @@ static void unique_in_pack(int len,\n \t}\n }\n \n+struct disambiguate_data {\n+\tint len;\n+\tconst unsigned char *bin_pfx;\n+\tstruct disambiguate_state *ds;\n+};\n+\n+static int find_short_packed_object_fn(struct packed_git *p, void *data)\n+{\n+\tstruct disambiguate_data *d = (struct disambiguate_data *) data;\n+\tunique_in_pack(d->len, d->bin_pfx, p, d->ds);\n+\treturn 0;\n+}\n+\n static void find_short_packed_object(int len, const unsigned char *bin_pfx,\n \t\t\t\t     struct disambiguate_state *ds)\n {\n-\tstruct packed_git *p;\n+\tstruct disambiguate_data d = {len,bin_pfx,ds};\n \n \tprepare_packed_git();\n-\tfor (p = packed_git; p && !ds->ambiguous; p = p->next)\n-\t\tunique_in_pack(len, bin_pfx, p, ds);\n+\tforeach_packed_git(find_short_packed_object_fn, NULL, &d);\n }\n \n #define SHORT_NAME_NOT_FOUND (-1)\n-- \n1.9.0.rc3.10.gb4796d9\n"},{"id":"234689","messageId":"xmqqmwhwm5yj.fsf@gitster.dls.corp.google.com","threadId":"35853","inReplyTo":"52fbc2d6.7kJdh2GOdWTzNcPK%szager@chromium.org","subject":"Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T20:28:04Z","receivedAt":"2014-02-12T20:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I'll locally fix up these style issues before commenting on the patch.\n\nThanks.\n\nERROR: space required after that ',' (ctx:VxV)\n#78: FILE: builtin/count-objects.c:111:\n+\tstruct pack_data pd = {0,0,0};\n \t                        ^\n\nERROR: space required after that ',' (ctx:VxV)\n#78: FILE: builtin/count-objects.c:111:\n+\tstruct pack_data pd = {0,0,0};\n \t                          ^\n\nERROR: space required after that ',' (ctx:VxV)\n#171: FILE: builtin/fsck.c:683:\n+\t\tstruct verify_packs_data vpd = {0,0,0};\n \t\t                                 ^\n\nERROR: space required after that ',' (ctx:VxV)\n#171: FILE: builtin/fsck.c:683:\n+\t\tstruct verify_packs_data vpd = {0,0,0};\n \t\t                                   ^\n\nERROR: space required after that ',' (ctx:VxV)\n#541: FILE: builtin/pack-redundant.c:591:\n+\tstruct add_pack_data apd = {filename,0,NULL};\n \t                                    ^\n\nERROR: space required after that ',' (ctx:VxV)\n#541: FILE: builtin/pack-redundant.c:591:\n+\tstruct add_pack_data apd = {filename,0,NULL};\n \t                                      ^\n\nERROR: space required after that ',' (ctx:VxV)\n#565: FILE: builtin/pack-redundant.c:604:\n+\tstruct add_pack_data apd = {NULL,0,NULL};\n \t                                ^\n\nERROR: space required after that ',' (ctx:VxV)\n#565: FILE: builtin/pack-redundant.c:604:\n+\tstruct add_pack_data apd = {NULL,0,NULL};\n \t                                  ^\n\nERROR: space prohibited after that '-' (ctx:WxW)\n#726: FILE: pack-revindex.c:47:\n+\tnum = - 1 - num;\n \t      ^\n\nERROR: space required after that ',' (ctx:VxV)\n#901: FILE: sha1_name.c:195:\n+\tstruct disambiguate_data d = {len,bin_pfx,ds};\n \t                                 ^\n\nERROR: space required after that ',' (ctx:VxV)\n#901: FILE: sha1_name.c:195:\n+\tstruct disambiguate_data d = {len,bin_pfx,ds};\n \t                                         ^\n\ntotal: 11 errors, 0 warnings, 781 lines checked\n"},{"id":"234692","messageId":"xmqqioskm3l6.fsf@gitster.dls.corp.google.com","threadId":"35853","inReplyTo":"52fbc2d6.7kJdh2GOdWTzNcPK%szager@chromium.org","subject":"Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-12T21:19:17Z","receivedAt":"2014-02-12T21:19:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"szager@chromium.org writes:\n\n> From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001\n\nPlease drop this line.\n\n> From: Stefan Zager <szager@chromium.org>\n\nPlease drop this line and instead have it in your e-mail header.\n\n> Date: Mon, 10 Feb 2014 16:55:12 -0800\n\nThe date in your e-mail header, which is the first time general\npublic saw this particular version of the patch, is used by default\nas the author time.  Unless there is a compelling reason to override\nthat with an in-body header, please drop this line.\n\n> Subject: [PATCH] Make the global packed_git variable static to sha1_file.c.\n\nPlease drop this line and instead have it in your e-mail header.\n\n> This patch is a pure refactor with no functional changes,...\n>\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index a7f70cb..6554dfe 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -83,14 +83,32 @@ static char const * const count_objects_usage[] = {\n>  \tNULL\n>  };\n>  \n> +struct pack_data {\n> +\tunsigned long packed;\n> +\toff_t size_pack;\n> +\tunsigned long num_pack;\n> +};\n> +\n> +int pack_data_fn(struct packed_git *p, void *data)\n\nCan't/shouldn't this be static?\n\nAlso I'd suggest s/pack_data_fn/collect_pack_data/ or something.\n\"_fn\" may be a good suffix for typedef'ed typename used in a\ncallback function, but for a concrete function, it only adds noise\nwithout giving us anything new.\n\n\tYes, I know there are a few existing violators, but we\n\tshouldn't make the codebase worse, using their existence due\n\tto past carelessness as an excuse.\n\n> diff --git a/cache.h b/cache.h\n> index dc040fb..542a9d9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> ...\n> +/* The 'hint' argument is for the commonly-used 'last found pack' optimization.\n> + * It can be NULL.\n> + */\n\n/*\n * Please try to have opening slash-asterisk and closing\n * asterisk-slash in a multi-line comment block on their\n * own lines by themselves.\n */\n\n> +extern void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint, void *data);\n> +\n> +extern size_t packed_git_count();\n> +extern size_t packed_git_local_count();\n\nextern size_t packed_git_count(void);\nextern size_t packed_git_local_count(void);\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 6e8c05d..aeeb7e6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -60,6 +60,7 @@ static struct cached_object empty_tree = {\n>  \t0\n>  };\n>  \n> +static struct packed_git *packed_git;\n>  static struct packed_git *last_found_pack;\n>  \n>  static struct cached_object *find_cached_object(const unsigned char *sha1)\n> @@ -468,7 +469,6 @@ static unsigned int pack_open_fds;\n>  static unsigned int pack_max_fds;\n>  static size_t peak_pack_mapped;\n>  static size_t pack_mapped;\n> -struct packed_git *packed_git;\n\nHmm, any particular reason why only this variable and not others are\nmoved up?\n\n> @@ -1091,6 +1091,37 @@ struct packed_git *add_packed_git(const char *path, int path_len, int local)\n>  \treturn p;\n>  }\n>  \n> +void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint, void *data)\n> +{\n> +\tstruct packed_git *p;\n> +\tif (hint && ((*fn)(hint, data)))\n> +\t\treturn;\n> +\tfor (p = packed_git; p; p = p->next)\n> +\t\tif (p != hint && (*fn)(p, data))\n> +\t\t\treturn;\n\n(mental note) In the new API, a non-zero return signals an early\nreturn/break from the loop.\n\n> +}\n> +\n> +size_t packed_git_count()\n\nsize_t packed_git_count(void)\n\n> +{\n> +\tsize_t res = 0;\n> +\tstruct packed_git *p;\n> +\n> +\tfor (p = packed_git; p; p = p->next)\n> +\t\t++res;\n\nWhen pre- or post- increment does not make any difference (i.e. you\ndo not use its value), please stick to post-increment.  pre-increment\nlooks unusual in our codebase and becomes distracting while reading\nit through.\n\nSame comments for packed-git-local-count.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 541667f..bc3074b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -900,14 +900,45 @@ static int no_try_delta(const char *path)\n>  \treturn 0;\n>  }\n>  \n> +struct find_pack_data {\n> +\tconst unsigned char *sha1;\n> +\toff_t offset;\n> +\tstruct packed_git *found_pack;\n> +\tint exclude;\n> +\tint found_non_local_pack;\n> +\tint found_pack_keep;\n> +};\n> +\n> +static int find_pack_fn(struct packed_git *p, void *data)\n> +{\n> +\tstruct find_pack_data *fpd = (struct find_pack_data *) data;\n> +\toff_t offset = find_pack_entry_one(fpd->sha1, p);\n> +\tif (offset) {\n> +\t\tif (!fpd->found_pack) {\n> +\t\t\tif (!is_pack_valid(p)) {\n> +\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t\tfpd->offset = offset;\n> +\t\t\tfpd->found_pack = p;\n> +\t\t}\n> +\t\tif (fpd->exclude)\n> +\t\t\treturn 1;\n> +\t\tif (!p->pack_local)\n> +\t\t\tfpd->found_non_local_pack = 1;\n> +\t\telse if (p->pack_keep)\n> +\t\t\tfpd->found_pack_keep = 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n>  static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>  \t\t\t    const char *name, int exclude)\n>  {\n>  \tstruct object_entry *entry;\n> -\tstruct packed_git *p, *found_pack = NULL;\n> -\toff_t found_offset = 0;\n>  \tint ix;\n>  \tuint32_t hash = name_hash(name);\n> +\tstruct find_pack_data fpd;\n>  \n>  \tix = nr_objects ? locate_object_entry_hash(sha1) : -1;\n>  \tif (ix >= 0) {\n> @@ -923,27 +954,18 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>  \tif (!exclude && local && has_loose_object_nonlocal(sha1))\n>  \t\treturn 0;\n>  \n> -\tfor (p = packed_git; p; p = p->next) {\n> -\t\toff_t offset = find_pack_entry_one(sha1, p);\n> -\t\tif (offset) {\n> -\t\t\tif (!found_pack) {\n> -\t\t\t\tif (!is_pack_valid(p)) {\n> -\t\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n> -\t\t\t\t\tcontinue;\n> -\t\t\t\t}\n> -\t\t\t\tfound_offset = offset;\n> -\t\t\t\tfound_pack = p;\n> -\t\t\t}\n> -\t\t\tif (exclude)\n> -\t\t\t\tbreak;\n> -\t\t\tif (incremental)\n> -\t\t\t\treturn 0;\n> -\t\t\tif (local && !p->pack_local)\n> -\t\t\t\treturn 0;\n> -\t\t\tif (ignore_packed_keep && p->pack_local && p->pack_keep)\n> -\t\t\t\treturn 0;\n> -\t\t}\n> -\t}\n> +\tfpd.sha1 = sha1;\n> +\tfpd.offset = 0;\n> +\tfpd.found_pack = NULL;\n> +\tfpd.exclude = exclude;\n> +\tfpd.found_non_local_pack = fpd.found_pack_keep = 0;\n\nI am debating myself if this should be the usual\n\n\tmemset(&fpd, 0, sizeof(fpd);\n\nfollowed by assignments to the fields that matter.\n\n> +\tforeach_packed_git(find_pack_fn, NULL, &fpd);\n> +\tif (fpd.found_pack && !exclude &&\n> +\t    (incremental ||\n> +\t     (local && fpd.found_non_local_pack) ||\n> +\t     (ignore_packed_keep && fpd.found_pack_keep)))\n> +\t\treturn 0;\n\nWhen told to do --incremental, we used to return 0 from this\nfunction immediately once we find the object in one pack, without\ngoing thru the list of packs.  Now we let foreach to loop thru all\nof them and then return 0.  Does this difference matter?  A similar\ndifference may exist for local/keep but I did not think it through.\n\nI'd stop here for now.  Thanks for starting this.\n"},{"id":"234752","messageId":"CAHOQ7J_06UBEgitY7G8DTwvWToq2qfVyRYrzdBos4bsL7i7oew@mail.gmail.com","threadId":"35853","inReplyTo":"xmqqioskm3l6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Make the global packed_git variable static to sha1_file.c.","fromName":"Stefan Zager","fromEmail":"szager@chromium.org","sentAt":"2014-02-13T23:15:06Z","receivedAt":"2014-02-13T23:15:06Z","isPatch":true,"sender":{"key":"szager@chromium.org","avatar":null},"body":"I uploaded a new patch; a few comments inline below...\n\nOn Wed, Feb 12, 2014 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> szager@chromium.org writes:\n>\n> Also I'd suggest s/pack_data_fn/collect_pack_data/ or something.\n> \"_fn\" may be a good suffix for typedef'ed typename used in a\n> callback function, but for a concrete function, it only adds noise\n> without giving us anything new.\n\nFixed this everywhere.\n\n>>  static struct cached_object *find_cached_object(const unsigned char *sha1)\n>> @@ -468,7 +469,6 @@ static unsigned int pack_open_fds;\n>>  static unsigned int pack_max_fds;\n>>  static size_t peak_pack_mapped;\n>>  static size_t pack_mapped;\n>> -struct packed_git *packed_git;\n>\n> Hmm, any particular reason why only this variable and not others are\n> moved up?\n\nNo, just need packed_git declared before use.  I moved all the static\nvariables up, for clarity.\n\n>> +     foreach_packed_git(find_pack_fn, NULL, &fpd);\n>> +     if (fpd.found_pack && !exclude &&\n>> +         (incremental ||\n>> +          (local && fpd.found_non_local_pack) ||\n>> +          (ignore_packed_keep && fpd.found_pack_keep)))\n>> +             return 0;\n>\n> When told to do --incremental, we used to return 0 from this\n> function immediately once we find the object in one pack, without\n> going thru the list of packs.  Now we let foreach to loop thru all\n> of them and then return 0.  Does this difference matter?  A similar\n> difference may exist for local/keep but I did not think it through.\n\nFixed.\n\n\n\nThanks,\n\nStefan\n"}]}