{"thread":{"id":"12011","subject":"[RFC Patch] Preventing corrupt objects from entering the repository","startedAt":"2008-02-10T17:58:12Z","lastAt":"2008-02-15T07:18:00Z","messageCount":23,"participants":["Martin Koegler","Junio C Hamano","Nicolas Pitre","Shawn O. Pearce","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"68241","messageId":"20080210175812.GB12162@auto.tuwien.ac.at","threadId":"12011","inReplyTo":null,"subject":"[RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-10T17:58:12Z","receivedAt":"2008-02-10T17:58:12Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Some weeks ago, I ask myself, what unexpected things could be\ndone by user via git-shell/git-daemon. I created logical corrupt \nobjects (such can be uploaded via receive pack without any problem)\nand got git to segfaults very quickly.\n\nIn most cases they are cause by one of the two things:\n\n* lookup/parse_XXX returns a NULL pointer, which is not handled \n in many cases.\n* The return values of functions are not checked.\n\nFixing all these things would be a lot of work and maybe even slow\ndown git. Even if all this things would be fixed, a corrupt \nobject in the history would mean, that git-upload-pack will die \nwith an error message and that a admin must fix the repository.\n\nSo I tried to fix the problem in another way: Instead of coping with \ncorrupt objects, stop them from entering the repository.\n\nThese patch series does the following things:\n* add a type aware object chain walker for all child objects (patch 2)\n\n I have not found a similar function in the git core, so I wrote\n one. I'm open for better alternatives.\n\n* move fsck to object chain walker (and get rid of track objects) (patch 3,4,5,12)\n\n I'm not sure, if this is advocated by others. At least it fixes \n some crashes in the case of NULL pointers.\n\n* Factor out reuseable code from builtin-fsck.c to fsck.c (patch 6+7)\n* add --strict option to unpack-objects (patch 1,8,9)\n\n The --strict option changes writing order slightly. All non blob objects\n are not written out immediatly. After the last object of the packfile\n is read, unpack-objects starts looking for non written objects. If the object\n is not corrupt and all its links are available, the object is written.\n\n This way only dangling objects can be created. \n\n On shallow repositories, this check can fail, if the list of shallow commits\n is changed. For the intended use in receive-pack, this is not a problem, as\n it does not work on such repositories.\n\n* add --strict option to index-pack (patch 8,10)\n\n Same as for unpack-objects, but without writting objects.\n\n* add config option for receive pack to enable checking (patch 11)\n\nThese patch serie is a first prototype to get some feedback.\n\nmfg Martin Kögler\n\nPatch 1: unpack-object: cache for non written objects\nPatch 2: add generic, type aware object chain walker\nPatch 3: fsck: move mark-reachable to fsck_walk\nPatch 4: fsck: move reachable object check to fsck_walk\nPatch 5: fsck: disable track objects\nPatch 6: create a common object checker code out of fsck\nPatch 7: fsck: use common object checker\nPatch 8: add common fsck error printing function\nPatch 9: unpack-objects: prevent writing of inconsistent objects\nPatch 10: index-pack: introduce checking mode\nPatch 11: receive-pack: use strict mode for unpacking objects\nPatch 12: Remove unused object-ref code\n\n\n Documentation/config.txt | 6 +\n Documentation/git-index-pack.txt | 3 +\n Documentation/git-unpack-objects.txt | 3 +\n Makefile | 6 +-\n builtin-fetch-pack.c | 1 -\n builtin-fsck.c | 329 +++++++++-------------------------\n builtin-pack-objects.c | 1 -\n builtin-rev-list.c | 1 -\n builtin-unpack-objects.c | 131 +++++++++++++-\n commit.c | 11 --\n fsck.c | 314 ++++++++++++++++++++++++++++++++\n fsck.h | 16 ++\n index-pack.c | 78 ++++++++-\n object-refs.c | 87 ---------\n object.h | 8 -\n receive-pack.c | 36 +++--\n tag.c | 6 -\n tree.c | 48 -----\n upload-pack.c | 1 -\n walker.c | 1 -\n 20 files changed, 653 insertions(+), 434 deletions(-)\n\n------------------------------------------------------------------------------\n\nFrom 76e86fe55345e633c910d6b8fe166e27c23c5aaf Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 08:51:38 +0100\nSubject: [PATCH 01/12] unpack-object: cache for non written objects\n\nPreventing objects with broken links entering the repository\nmeans, that write of some objects must be delayed.\n\nThis patch adds a cache to keep the object data in memory. The delta\nresolving code must also search in the cache.\n\nSigned-off-by: Martin Koegler \n---\n builtin-unpack-objects.c | 32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 1e51865..f18c7e8 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -8,6 +8,7 @@\n #include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\n+#include \"decorate.h\"\n \n static int dry_run, quiet, recover, has_errors;\n static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n@@ -18,6 +19,28 @@ static unsigned int offset, len;\n static off_t consumed_bytes;\n static SHA_CTX ctx;\n \n+struct obj_buffer {\n+\tchar *buffer;\n+\tunsigned long size;\n+};\n+\n+static struct decoration obj_decorate;\n+\n+static struct obj_buffer *lookup_object_buffer(struct object *base)\n+{\n+\treturn lookup_decoration(&obj_decorate, base);\n+}\n+\n+static void add_object_buffer(struct object *object, char *buffer, unsigned long size)\n+{\n+\tstruct obj_buffer *obj;\n+\tobj = xcalloc(1, sizeof(struct obj_buffer));\n+\tobj->buffer = buffer;\n+\tobj->size = size;\n+\tif (add_decoration(&obj_decorate, object, obj))\n+\t\tdie(\"object %s tried to add buffer twice!\", sha1_to_hex(object->sha1));\n+}\n+\n /*\n * Make sure at least \"min\" bytes are available in the buffer, and\n * return the pointer to the buffer.\n@@ -252,6 +275,15 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t}\n \t}\n \n+\tstruct object* obj = lookup_object(base_sha1);\n+\tif (obj) {\n+\t\tstruct obj_buffer *obj_buf = lookup_object_buffer (obj);\n+\t\tif (obj_buf) {\n+\t\t\tresolve_delta(nr, obj->type, obj_buf->buffer, obj_buf->size, delta_data, delta_size);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\n \tbase = read_sha1_file(base_sha1, &type, &base_size);\n \tif (!base) {\n \t\terror(\"failed to read delta-pack base object %s\",\n-- \n1.5.4.g074be\n\n\nFrom 9fb732d8939ea2108440a0ab468aab15c648dde5 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:01:01 +0100\nSubject: [PATCH 02/12] add generic, type aware object chain walker\n\nThe requirements are:\n* it may not crash on NULL pointers\n* a callback function is needed, as index-pack/unpack-objects\n need to do different things\n* the type information is needed to check the expected <-> real type\n and print better error messages\n\nSigned-off-by: Martin Koegler \n---\n Makefile | 4 +-\n fsck.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n fsck.h | 10 +++++++\n 3 files changed, 96 insertions(+), 2 deletions(-)\n create mode 100644 fsck.c\n create mode 100644 fsck.h\n\ndiff --git a/Makefile b/Makefile\nindex 728cfc2..0921484 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -301,7 +301,7 @@ LIB_H = \\\n \trun-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \\\n \ttree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \\\n \tutf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \\\n-\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h\n+\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h fsck.h\n \n DIFF_OBJS = \\\n \tdiff.o diff-lib.o diffcore-break.o diffcore-order.o \\\n@@ -324,7 +324,7 @@ LIB_OBJS = \\\n \talloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \\\n \tcolor.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \\\n \tconvert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \\\n-\ttransport.o bundle.o walker.o parse-options.o ws.o archive.o\n+\ttransport.o bundle.o walker.o parse-options.o ws.o archive.o fsck.o\n \n BUILTIN_OBJS = \\\n \tbuiltin-add.o \\\ndiff --git a/fsck.c b/fsck.c\nnew file mode 100644\nindex 0000000..089f775\n--- /dev/null\n+++ b/fsck.c\n@@ -0,0 +1,84 @@\n+#include \"cache.h\"\n+#include \"object.h\"\n+#include \"blob.h\"\n+#include \"tree.h\"\n+#include \"tree-walk.h\"\n+#include \"commit.h\"\n+#include \"tag.h\"\n+#include \"fsck.h\"\n+\n+static int fsck_walk_tree(struct tree* tree, fsck_walk_func walk, void* data)\n+{\n+\tstruct tree_desc desc;\n+\tstruct name_entry entry;\n+\n+\tif(parse_tree(tree))\n+\t\treturn -1;\n+\n+\tinit_tree_desc(&desc, tree->buffer, tree->size);\n+\twhile(tree_entry(&desc, &entry)) {\n+\t\tint result;\n+\t\t\n+\t\tif (S_ISGITLINK(entry.mode))\n+\t\t\tcontinue;\n+\t\tif (S_ISDIR(entry.mode))\n+\t\t\tresult = walk(&lookup_tree(entry.sha1)->object, OBJ_TREE, data);\n+\t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode))\n+\t\t\tresult = walk(&lookup_blob(entry.sha1)->object, OBJ_BLOB, data);\n+\t\telse {\n+\t\t\terror(\"in tree %s: entry %s has bad mode %.6o\\n\",\n+\t\t\t sha1_to_hex(tree->object.sha1), entry.path, entry.mode);\n+\t\t\tresult = -1;\n+\t\t}\n+\t\tif (result)\n+\t\t\treturn result;\n+\t}\n+\treturn 0;\n+}\n+\n+static int fsck_walk_commit(struct commit* commit, fsck_walk_func walk, void* data)\n+{\n+\tstruct commit_list *parents = commit->parents;\n+\tint result;\n+\n+\tif(parse_commit(commit))\n+\t\treturn -1;\n+\n+\tresult = walk((struct object*)commit->tree, OBJ_TREE, data);\n+\tif (result)\n+\t\treturn result;\n+\n+\twhile (parents) {\n+\t\tresult = walk((struct object*)parents->item, OBJ_COMMIT, data);\n+\t\tif (result)\n+\t\t\treturn result;\n+\t\tparents = parents->next;\n+\t}\n+\treturn 0;\n+}\n+\n+static int fsck_walk_tag(struct tag* tag, fsck_walk_func walk, void* data)\n+{\n+\tif(parse_tag(tag))\n+\t\treturn -1;\n+\treturn walk(tag->tagged, OBJ_ANY, data);\n+}\n+\n+int fsck_walk(struct object* obj, fsck_walk_func walk, void* data)\n+{\n+\tif (!obj)\n+\t\treturn -1;\n+\tswitch(obj->type) {\n+\tcase OBJ_BLOB:\n+\t\treturn 0;\n+\tcase OBJ_TREE:\n+\t\treturn fsck_walk_tree((struct tree*)obj, walk, data);\n+\tcase OBJ_COMMIT:\n+\t\treturn fsck_walk_commit((struct commit*)obj, walk, data);\n+\tcase OBJ_TAG:\n+\t\treturn fsck_walk_tag((struct tag*)obj, walk, data);\n+\tdefault:\n+\t\terror(\"Unknown object type for %s\", sha1_to_hex(obj->sha1));\n+\t\treturn -1;\n+\t}\n+}\ndiff --git a/fsck.h b/fsck.h\nnew file mode 100644\nindex 0000000..caeb2c9\n--- /dev/null\n+++ b/fsck.h\n@@ -0,0 +1,10 @@\n+#ifndef GIT_FSCK_H\n+#define GIT_FSCK_H\n+\n+#define OBJ_ANY OBJ_BAD\n+\n+typedef int (*fsck_walk_func)(struct object* obj, int type, void* data);\n+\n+int fsck_walk(struct object* obj, fsck_walk_func walk, void* data);\n+\n+#endif\n-- \n1.5.4.g074be\n\n\nFrom 80b22c3f2c3e13c207790a49646020c55b34bba7 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:01:50 +0100\nSubject: [PATCH 03/12] fsck: move mark-reachable to fsck_walk\n\nSigned-off-by: Martin Koegler \n---\n builtin-fsck.c | 34 ++++++++++++++++++++++++----------\n 1 files changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex cc7524b..49e96ff 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -8,6 +8,7 @@\n #include \"pack.h\"\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\n+#include \"fsck.h\"\n #include \"parse-options.h\"\n \n #define REACHABLE 0x0001\n@@ -35,6 +36,23 @@ static int verbose;\n #define DIRENT_SORT_HINT(de) ((de)->d_ino)\n #endif\n \n+static int mark_object(struct object* obj, int type, void* data)\n+{\n+\tif (!obj)\n+\t\treturn 0;\n+\tif (obj->flags & REACHABLE)\n+\t\treturn 0;\n+\tobj->flags |= REACHABLE;\n+\tif (!obj->parsed)\n+\t\treturn 0;\n+\treturn fsck_walk(obj, mark_object, data);\n+}\n+\n+static void mark_object_reachable(struct object* obj)\n+{\n+\tmark_object(obj, 0, 0);\n+}\n+\n static void objreport(struct object *obj, const char *severity,\n const char *err, va_list params)\n {\n@@ -326,8 +344,6 @@ static int fsck_tree(struct tree *item)\n \t\to_name = name;\n \t\to_sha1 = sha1;\n \t}\n-\tfree(item->buffer);\n-\titem->buffer = NULL;\n \n \tretval = 0;\n \tif (has_full_path) {\n@@ -375,8 +391,6 @@ static int fsck_commit(struct commit *commit)\n \t}\n \tif (memcmp(buffer, \"author \", 7))\n \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n \tif (!commit->tree)\n \t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n \tif (!commit->parents && show_root)\n@@ -538,13 +552,13 @@ static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \t\tobj = lookup_object(osha1);\n \t\tif (obj) {\n \t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n+\t\t\tmark_object_reachable(obj);\n \t\t}\n \t}\n \tobj = lookup_object(nsha1);\n \tif (obj) {\n \t\tobj->used = 1;\n-\t\tmark_reachable(obj, REACHABLE);\n+\t\tmark_object_reachable(obj);\n \t}\n \treturn 0;\n }\n@@ -574,7 +588,7 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f\n \t\terror(\"%s: not a commit\", refname);\n \tdefault_refs++;\n \tobj->used = 1;\n-\tmark_reachable(obj, REACHABLE);\n+\tmark_object_reachable(obj);\n \n \treturn 0;\n }\n@@ -660,7 +674,7 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\t sha1_to_hex(it->sha1));\n \t\t\treturn 1;\n \t\t}\n-\t\tmark_reachable(obj, REACHABLE);\n+\t\tmark_object_reachable(obj);\n \t\tobj->used = 1;\n \t\tif (obj->type != OBJ_TREE)\n \t\t\terr |= objerror(obj, \"non-tree in cache-tree\");\n@@ -741,7 +755,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \n \t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n+\t\t\tmark_object_reachable(obj);\n \t\t\theads++;\n \t\t\tcontinue;\n \t\t}\n@@ -773,7 +787,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tobj = &blob->object;\n \t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n+\t\t\tmark_object_reachable(obj);\n \t\t}\n \t\tif (active_cache_tree)\n \t\t\tfsck_cache_tree(active_cache_tree);\n-- \n1.5.4.g074be\n\n\nFrom ce43251ef71962ff64fe138f1295c405ef6aaf65 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:04:08 +0100\nSubject: [PATCH 04/12] fsck: move reachable object check to fsck_walk\n\nIt handles NULL pointers in object references without crashing.\n\nSigned-off-by: Martin Koegler \n---\n builtin-fsck.c | 49 +++++++++++++++++++++++++++++--------------------\n 1 files changed, 29 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 49e96ff..2c1e10f 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -81,13 +81,39 @@ static int objwarning(struct object *obj, const char *err, ...)\n \treturn -1;\n }\n \n+static int check_reachable_object_childs(struct object *obj, int type, void *data)\n+{\n+\tstruct object *parent = data;\n+\tif (!obj) {\n+\t\tprintf(\"broken link from %7s %s\\n\",\n+\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n+\t\tprintf(\"broken link from %7s %s\\n\",\n+\t\t\t (type==OBJ_ANY?\"unknown\":typename(type)), \"unknown\");\n+\t\terrors_found |= ERROR_REACHABLE;\n+\t\treturn 0;\n+\t}\n+\n+\tif (type != OBJ_ANY && obj->type != type) {\n+\t\tobjerror(parent, \"wrong object type in link\");\n+\t\treturn 0;\n+\t}\n+\n+\tif (obj->parsed || (has_sha1_file(obj->sha1)))\n+\t\treturn 0;\n+\t\t\t\t\n+\tprintf(\"broken link from %7s %s\\n\",\n+\t typename(parent->type), sha1_to_hex(parent->sha1));\n+\tprintf(\" to %7s %s\\n\",\n+\t typename(obj->type), sha1_to_hex(obj->sha1));\n+\terrors_found |= ERROR_REACHABLE;\n+\treturn 0;\n+}\n+\n /*\n * Check a single reachable object\n */\n static void check_reachable_object(struct object *obj)\n {\n-\tconst struct object_refs *refs;\n-\n \t/*\n \t * We obviously want the object to be parsed,\n \t * except if it was in a pack-file and we didn't\n@@ -101,24 +127,7 @@ static void check_reachable_object(struct object *obj)\n \t\treturn;\n \t}\n \n-\t/*\n-\t * Check that everything that we try to reference is also good.\n-\t */\n-\trefs = lookup_object_refs(obj);\n-\tif (refs) {\n-\t\tunsigned j;\n-\t\tfor (j = 0; j < refs->count; j++) {\n-\t\t\tstruct object *ref = refs->ref[j];\n-\t\t\tif (ref->parsed ||\n-\t\t\t (has_sha1_file(ref->sha1)))\n-\t\t\t\tcontinue;\n-\t\t\tprintf(\"broken link from %7s %s\\n\",\n-\t\t\t typename(obj->type), sha1_to_hex(obj->sha1));\n-\t\t\tprintf(\" to %7s %s\\n\",\n-\t\t\t typename(ref->type), sha1_to_hex(ref->sha1));\n-\t\t\terrors_found |= ERROR_REACHABLE;\n-\t\t}\n-\t}\n+\tfsck_walk(obj, check_reachable_object_childs, obj);\n }\n \n /*\n-- \n1.5.4.g074be\n\n\nFrom ba836cfe0202f97bd5901e1155abfd22b698020c Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:06:02 +0100\nSubject: [PATCH 05/12] fsck: disable track objects\n\nMark the usage of object with fsck_walk. As this is the last user\nof object_refs, it is disabled.\n\nSigned-off-by: Martin Koegler \n---\n builtin-fsck.c | 10 +++++++++-\n 1 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 2c1e10f..b0ca3ad 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -53,6 +53,14 @@ static void mark_object_reachable(struct object* obj)\n \tmark_object(obj, 0, 0);\n }\n \n+static int mark_used(struct object* obj, int type, void* data)\n+{\n+\tif (!obj)\n+\t\treturn 0;\n+\tobj->used = 1;\n+\treturn 0;\n+}\n+\n static void objreport(struct object *obj, const char *severity,\n const char *err, va_list params)\n {\n@@ -437,6 +445,7 @@ static int fsck_sha1(const unsigned char *sha1)\n \tif (obj->flags & SEEN)\n \t\treturn 0;\n \tobj->flags |= SEEN;\n+\tfsck_walk (obj, mark_used, 0);\n \tif (obj->type == OBJ_BLOB)\n \t\treturn 0;\n \tif (obj->type == OBJ_TREE)\n@@ -716,7 +725,6 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n {\n \tint i, heads;\n \n-\ttrack_object_refs = 1;\n \terrors_found = 0;\n \n \targc = parse_options(argc, argv, fsck_opts, fsck_usage, 0);\n-- \n1.5.4.g074be\n\n\nFrom ee11f771be1ef1c29725cb56ab3eb8dfe61ca25a Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:07:33 +0100\nSubject: [PATCH 06/12] create a common object checker code out of fsck\n\nThe function provides a callback for reporting errors.\n\nSigned-off-by: Martin Koegler \n---\n fsck.c | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n fsck.h | 4 +\n 2 files changed, 204 insertions(+), 0 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 089f775..ac253ae 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -82,3 +82,203 @@ int fsck_walk(struct object* obj, fsck_walk_func walk, void* data)\n \t\treturn -1;\n \t}\n }\n+\n+/*\n+ * The entries in a tree are ordered in the _path_ order,\n+ * which means that a directory entry is ordered by adding\n+ * a slash to the end of it.\n+ *\n+ * So a directory called \"a\" is ordered _after_ a file\n+ * called \"a.c\", because \"a/\" sorts after \"a.c\".\n+ */\n+#define TREE_UNORDERED (-1)\n+#define TREE_HAS_DUPS (-2)\n+\n+static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, const char *name2)\n+{\n+\tint len1 = strlen(name1);\n+\tint len2 = strlen(name2);\n+\tint len = len1 < len2 ? len1 : len2;\n+\tunsigned char c1, c2;\n+\tint cmp;\n+\n+\tcmp = memcmp(name1, name2, len);\n+\tif (cmp < 0)\n+\t\treturn 0;\n+\tif (cmp > 0)\n+\t\treturn TREE_UNORDERED;\n+\n+\t/*\n+\t * Ok, the first characters are the same.\n+\t * Now we need to order the next one, but turn\n+\t * a '\\0' into a '/' for a directory entry.\n+\t */\n+\tc1 = name1[len];\n+\tc2 = name2[len];\n+\tif (!c1 && !c2)\n+\t\t/*\n+\t\t * git-write-tree used to write out a nonsense tree that has\n+\t\t * entries with the same name, one blob and one tree. Make\n+\t\t * sure we do not have duplicate entries.\n+\t\t */\n+\t\treturn TREE_HAS_DUPS;\n+\tif (!c1 && S_ISDIR(mode1))\n+\t\tc1 = '/';\n+\tif (!c2 && S_ISDIR(mode2))\n+\t\tc2 = '/';\n+\treturn c1 < c2 ? 0 : TREE_UNORDERED;\n+}\n+\n+\n+static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n+{\n+\tint retval;\n+\tint has_full_path = 0;\n+\tint has_empty_name = 0;\n+\tint has_zero_pad = 0;\n+\tint has_bad_modes = 0;\n+\tint has_dup_entries = 0;\n+\tint not_properly_sorted = 0;\n+\tstruct tree_desc desc;\n+\tunsigned o_mode;\n+\tconst char *o_name;\n+\tconst unsigned char *o_sha1;\n+\n+\tinit_tree_desc(&desc, item->buffer, item->size);\n+\n+\to_mode = 0;\n+\to_name = NULL;\n+\to_sha1 = NULL;\n+\tif (!desc.size)\n+\t\treturn error_func (&item->object, FSCK_ERROR, \"empty tree\");\n+\n+\twhile (desc.size) {\n+\t\tunsigned mode;\n+\t\tconst char *name;\n+\t\tconst unsigned char *sha1;\n+\n+\t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n+\n+\t\tif (strchr(name, '/'))\n+\t\t\thas_full_path = 1;\n+\t\tif (!*name)\n+\t\t\thas_empty_name = 1;\n+\t\thas_zero_pad |= *(char *)desc.buffer == '0';\n+\t\tupdate_tree_entry(&desc);\n+\n+\t\tswitch (mode) {\n+\t\t/*\n+\t\t * Standard modes..\n+\t\t */\n+\t\tcase S_IFREG | 0755:\n+\t\tcase S_IFREG | 0644:\n+\t\tcase S_IFLNK:\n+\t\tcase S_IFDIR:\n+\t\tcase S_IFGITLINK:\n+\t\t\tbreak;\n+\t\t/*\n+\t\t * This is nonstandard, but we had a few of these\n+\t\t * early on when we honored the full set of mode\n+\t\t * bits..\n+\t\t */\n+\t\tcase S_IFREG | 0664:\n+\t\t\tif (!strict)\n+\t\t\t\tbreak;\n+\t\tdefault:\n+\t\t\thas_bad_modes = 1;\n+\t\t}\n+\n+\t\tif (o_name) {\n+\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n+\t\t\tcase TREE_UNORDERED:\n+\t\t\t\tnot_properly_sorted = 1;\n+\t\t\t\tbreak;\n+\t\t\tcase TREE_HAS_DUPS:\n+\t\t\t\thas_dup_entries = 1;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\to_mode = mode;\n+\t\to_name = name;\n+\t\to_sha1 = sha1;\n+\t}\n+\n+\tretval = 0;\n+\tif (has_full_path) {\n+\t\tretval += error_func(&item->object, FSCK_WARN, \"contains full pathnames\");\n+\t}\n+\tif (has_empty_name) {\n+\t\tretval += error_func(&item->object, FSCK_WARN, \"contains empty pathname\");\n+\t}\n+\tif (has_zero_pad) {\n+\t\tretval += error_func(&item->object, FSCK_WARN, \"contains zero-padded file modes\");\n+\t}\n+\tif (has_bad_modes) {\n+\t\tretval += error_func(&item->object, FSCK_WARN, \"contains bad file modes\");\n+\t}\n+\tif (has_dup_entries) {\n+\t\tretval += error_func(&item->object, FSCK_ERROR, \"contains duplicate file entries\");\n+\t}\n+\tif (not_properly_sorted) {\n+\t\tretval += error_func(&item->object, FSCK_ERROR, \"not properly sorted\");\n+\t}\n+\treturn retval;\n+}\n+\n+static int fsck_commit(struct commit *commit, fsck_error error_func)\n+{\n+\tchar *buffer = commit->buffer;\n+\tunsigned char tree_sha1[20], sha1[20];\n+\n+\tif (!commit->date)\n+\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line in %s\",\n+\t\t\t\t sha1_to_hex(commit->object.sha1));\n+\n+\tif (memcmp(buffer, \"tree \", 5))\n+\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n+\tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n+\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n+\tbuffer += 46;\n+\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n+\t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n+\t\tbuffer += 48;\n+\t}\n+\tif (memcmp(buffer, \"author \", 7))\n+\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n+\tif (!commit->tree)\n+\t\treturn error_func(&commit->object, FSCK_ERROR, \"could not load commit's tree %s\", sha1_to_hex(tree_sha1));\n+\n+\treturn 0;\n+}\n+\n+static int fsck_tag(struct tag *tag, fsck_error error_func)\n+{\n+\tstruct object *tagged = tag->tagged;\n+\n+\tif (!tagged) {\n+\t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n+\t}\n+\treturn 0;\n+}\n+\n+int fsck_object(struct object *obj, int strict, fsck_error error_func)\n+{\n+\tif (!obj)\n+\t\treturn error_func(obj, FSCK_ERROR, \"no object given\");\n+\n+\tif (obj->type == OBJ_BLOB)\n+\t\treturn 0;\n+\tif (obj->type == OBJ_TREE)\n+\t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n+\tif (obj->type == OBJ_COMMIT)\n+\t\treturn fsck_commit((struct commit *) obj, error_func);\n+\tif (obj->type == OBJ_TAG)\n+\t\treturn fsck_tag((struct tag *) obj, error_func);\n+\n+\treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n+\t\t\t obj->type);\n+}\ndiff --git a/fsck.h b/fsck.h\nindex caeb2c9..78819d3 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -2,9 +2,13 @@\n #define GIT_FSCK_H\n \n #define OBJ_ANY OBJ_BAD\n+#define FSCK_ERROR 1\n+#define FSCK_WARN 2\n \n typedef int (*fsck_walk_func)(struct object* obj, int type, void* data);\n+typedef int (*fsck_error)(struct object *obj, int type, const char *err, ...);\n \n int fsck_walk(struct object* obj, fsck_walk_func walk, void* data);\n+int fsck_object(struct object *obj, int strict, fsck_error error_func);\n \n #endif\n-- \n1.5.4.g074be\n\n\nFrom 22c621718c90d88c6295bd8035001451f9b7bf9d Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:07:59 +0100\nSubject: [PATCH 07/12] fsck: use common object checker\n\nSigned-off-by: Martin Koegler \n---\n builtin-fsck.c | 236 ++++++-------------------------------------------------\n 1 files changed, 26 insertions(+), 210 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex b0ca3ad..f69dc99 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -80,13 +80,13 @@ static int objerror(struct object *obj, const char *err, ...)\n \treturn -1;\n }\n \n-static int objwarning(struct object *obj, const char *err, ...)\n+static int fsck_error_func(struct object *obj, int type, const char *err, ...)\n {\n \tva_list params;\n \tva_start(params, err);\n-\tobjreport(obj, \"warning\", err, params);\n+\tobjreport(obj, (type == FSCK_WARN)?\"warning\":\"error\", err, params);\n \tva_end(params);\n-\treturn -1;\n+\treturn (type == FSCK_WARN)?0:1;\n }\n \n static int check_reachable_object_childs(struct object *obj, int type, void *data)\n@@ -239,201 +239,6 @@ static void check_connectivity(void)\n \t}\n }\n \n-/*\n- * The entries in a tree are ordered in the _path_ order,\n- * which means that a directory entry is ordered by adding\n- * a slash to the end of it.\n- *\n- * So a directory called \"a\" is ordered _after_ a file\n- * called \"a.c\", because \"a/\" sorts after \"a.c\".\n- */\n-#define TREE_UNORDERED (-1)\n-#define TREE_HAS_DUPS (-2)\n-\n-static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, const char *name2)\n-{\n-\tint len1 = strlen(name1);\n-\tint len2 = strlen(name2);\n-\tint len = len1 < len2 ? len1 : len2;\n-\tunsigned char c1, c2;\n-\tint cmp;\n-\n-\tcmp = memcmp(name1, name2, len);\n-\tif (cmp < 0)\n-\t\treturn 0;\n-\tif (cmp > 0)\n-\t\treturn TREE_UNORDERED;\n-\n-\t/*\n-\t * Ok, the first characters are the same.\n-\t * Now we need to order the next one, but turn\n-\t * a '\\0' into a '/' for a directory entry.\n-\t */\n-\tc1 = name1[len];\n-\tc2 = name2[len];\n-\tif (!c1 && !c2)\n-\t\t/*\n-\t\t * git-write-tree used to write out a nonsense tree that has\n-\t\t * entries with the same name, one blob and one tree. Make\n-\t\t * sure we do not have duplicate entries.\n-\t\t */\n-\t\treturn TREE_HAS_DUPS;\n-\tif (!c1 && S_ISDIR(mode1))\n-\t\tc1 = '/';\n-\tif (!c2 && S_ISDIR(mode2))\n-\t\tc2 = '/';\n-\treturn c1 < c2 ? 0 : TREE_UNORDERED;\n-}\n-\n-static int fsck_tree(struct tree *item)\n-{\n-\tint retval;\n-\tint has_full_path = 0;\n-\tint has_empty_name = 0;\n-\tint has_zero_pad = 0;\n-\tint has_bad_modes = 0;\n-\tint has_dup_entries = 0;\n-\tint not_properly_sorted = 0;\n-\tstruct tree_desc desc;\n-\tunsigned o_mode;\n-\tconst char *o_name;\n-\tconst unsigned char *o_sha1;\n-\n-\tif (verbose)\n-\t\tfprintf(stderr, \"Checking tree %s\\n\",\n-\t\t\t\tsha1_to_hex(item->object.sha1));\n-\n-\tinit_tree_desc(&desc, item->buffer, item->size);\n-\n-\to_mode = 0;\n-\to_name = NULL;\n-\to_sha1 = NULL;\n-\twhile (desc.size) {\n-\t\tunsigned mode;\n-\t\tconst char *name;\n-\t\tconst unsigned char *sha1;\n-\n-\t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n-\n-\t\tif (strchr(name, '/'))\n-\t\t\thas_full_path = 1;\n-\t\tif (!*name)\n-\t\t\thas_empty_name = 1;\n-\t\thas_zero_pad |= *(char *)desc.buffer == '0';\n-\t\tupdate_tree_entry(&desc);\n-\n-\t\tswitch (mode) {\n-\t\t/*\n-\t\t * Standard modes..\n-\t\t */\n-\t\tcase S_IFREG | 0755:\n-\t\tcase S_IFREG | 0644:\n-\t\tcase S_IFLNK:\n-\t\tcase S_IFDIR:\n-\t\tcase S_IFGITLINK:\n-\t\t\tbreak;\n-\t\t/*\n-\t\t * This is nonstandard, but we had a few of these\n-\t\t * early on when we honored the full set of mode\n-\t\t * bits..\n-\t\t */\n-\t\tcase S_IFREG | 0664:\n-\t\t\tif (!check_strict)\n-\t\t\t\tbreak;\n-\t\tdefault:\n-\t\t\thas_bad_modes = 1;\n-\t\t}\n-\n-\t\tif (o_name) {\n-\t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n-\t\t\tcase TREE_UNORDERED:\n-\t\t\t\tnot_properly_sorted = 1;\n-\t\t\t\tbreak;\n-\t\t\tcase TREE_HAS_DUPS:\n-\t\t\t\thas_dup_entries = 1;\n-\t\t\t\tbreak;\n-\t\t\tdefault:\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n-\n-\t\to_mode = mode;\n-\t\to_name = name;\n-\t\to_sha1 = sha1;\n-\t}\n-\n-\tretval = 0;\n-\tif (has_full_path) {\n-\t\tobjwarning(&item->object, \"contains full pathnames\");\n-\t}\n-\tif (has_empty_name) {\n-\t\tobjwarning(&item->object, \"contains empty pathname\");\n-\t}\n-\tif (has_zero_pad) {\n-\t\tobjwarning(&item->object, \"contains zero-padded file modes\");\n-\t}\n-\tif (has_bad_modes) {\n-\t\tobjwarning(&item->object, \"contains bad file modes\");\n-\t}\n-\tif (has_dup_entries) {\n-\t\tretval = objerror(&item->object, \"contains duplicate file entries\");\n-\t}\n-\tif (not_properly_sorted) {\n-\t\tretval = objerror(&item->object, \"not properly sorted\");\n-\t}\n-\treturn retval;\n-}\n-\n-static int fsck_commit(struct commit *commit)\n-{\n-\tchar *buffer = commit->buffer;\n-\tunsigned char tree_sha1[20], sha1[20];\n-\n-\tif (verbose)\n-\t\tfprintf(stderr, \"Checking commit %s\\n\",\n-\t\t\tsha1_to_hex(commit->object.sha1));\n-\n-\tif (!commit->date)\n-\t\treturn objerror(&commit->object, \"invalid author/committer line\");\n-\n-\tif (memcmp(buffer, \"tree \", 5))\n-\t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n-\tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n-\t\treturn objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n-\tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n-\t\t\treturn objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n-\t\tbuffer += 48;\n-\t}\n-\tif (memcmp(buffer, \"author \", 7))\n-\t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n-\tif (!commit->tree)\n-\t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n-\tif (!commit->parents && show_root)\n-\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n-\treturn 0;\n-}\n-\n-static int fsck_tag(struct tag *tag)\n-{\n-\tstruct object *tagged = tag->tagged;\n-\n-\tif (verbose)\n-\t\tfprintf(stderr, \"Checking tag %s\\n\",\n-\t\t\tsha1_to_hex(tag->object.sha1));\n-\n-\tif (!tagged) {\n-\t\treturn objerror(&tag->object, \"could not load tagged object\");\n-\t}\n-\tif (!show_tags)\n-\t\treturn 0;\n-\n-\tprintf(\"tagged %s %s\", typename(tagged->type), sha1_to_hex(tagged->sha1));\n-\tprintf(\" (%s) in %s\\n\", tag->tag, sha1_to_hex(tag->object.sha1));\n-\treturn 0;\n-}\n-\n static int fsck_sha1(const unsigned char *sha1)\n {\n \tstruct object *obj = parse_object(sha1);\n@@ -445,19 +250,30 @@ static int fsck_sha1(const unsigned char *sha1)\n \tif (obj->flags & SEEN)\n \t\treturn 0;\n \tobj->flags |= SEEN;\n+\n+\tif (verbose)\n+\t\tfprintf(stderr, \"Checking %s %s\\n\",\n+\t\t\ttypename(obj->type), sha1_to_hex(obj->sha1));\n+\n \tfsck_walk (obj, mark_used, 0);\n-\tif (obj->type == OBJ_BLOB)\n-\t\treturn 0;\n-\tif (obj->type == OBJ_TREE)\n-\t\treturn fsck_tree((struct tree *) obj);\n-\tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj);\n-\tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj);\n-\n-\t/* By now, parse_object() would've returned NULL instead. */\n-\treturn objerror(obj, \"unknown type '%d' (internal fsck error)\",\n-\t\t\tobj->type);\n+\tif (fsck_object (obj, check_strict, fsck_error_func))\n+\t\treturn -1;\n+\n+\tif (obj->type == OBJ_COMMIT) {\n+\t\tstruct commit *commit = (struct commit*) obj;\n+\t\tif (!commit->parents && show_root)\n+\t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n+\t}\n+\n+\tif (obj->type == OBJ_TAG) {\n+\t\tstruct tag *tag = (struct tag*) obj;\n+\t\tif (show_tags && tag->tagged) {\n+\t\t\tprintf(\"tagged %s %s\", typename(tag->tagged->type), sha1_to_hex(tag->tagged->sha1));\n+\t\t\tprintf(\" (%s) in %s\\n\", tag->tag, sha1_to_hex(tag->object.sha1));\n+\t\t}\n+\t}\n+\n+\treturn 0;\n }\n \n /*\n-- \n1.5.4.g074be\n\n\nFrom 09938f28e230a236dc48014ace6c8ed02ad33084 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:08:45 +0100\nSubject: [PATCH 08/12] add common fsck error printing function\n\nSigned-off-by: Martin Koegler \n---\n fsck.c | 30 ++++++++++++++++++++++++++++++\n fsck.h | 2 ++\n 2 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex ac253ae..07e48a8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -282,3 +282,33 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n \t\t\t obj->type);\n }\n+\n+int fsck_error_function(struct object *obj, int type, const char* fmt, ...)\n+{\n+\tva_list ap;\n+\tint len;\n+\tstruct strbuf sb;\n+\t\n+\tstrbuf_init(&sb, 0);\n+\tstrbuf_addf(&sb, \"object %s:\", sha1_to_hex(obj->sha1));\n+\t\n+\tva_start(ap, fmt);\n+\tlen = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);\n+\tva_end(ap);\n+\t\n+\tif (len < 0)\n+\t\tlen = 0;\n+\tif (len >= strbuf_avail(&sb)) {\n+\t\tstrbuf_grow(&sb, len + 2);\n+\t\tva_start(ap, fmt);\n+\t\tlen = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);\n+\t\tva_end(ap);\n+\t\tif (len >= strbuf_avail(&sb)) {\n+\t\t\tdie(\"this should not happen, your snprintf is broken\");\n+\t\t}\n+\t}\n+\t\n+\terror(sb.buf);\n+\tstrbuf_release(&sb);\n+\treturn 1;\n+}\ndiff --git a/fsck.h b/fsck.h\nindex 78819d3..f13631c 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -8,6 +8,8 @@\n typedef int (*fsck_walk_func)(struct object* obj, int type, void* data);\n typedef int (*fsck_error)(struct object *obj, int type, const char *err, ...);\n \n+int fsck_error_function(struct object *obj, int type, const char* fmt, ...);\n+\n int fsck_walk(struct object* obj, fsck_walk_func walk, void* data);\n int fsck_object(struct object *obj, int strict, fsck_error error_func);\n \n-- \n1.5.4.g074be\n\n\nFrom a8db4e754e717bac0b2462333d4145eac3452099 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:14:14 +0100\nSubject: [PATCH 09/12] unpack-objects: prevent writing of inconsistent objects\n\nThis patch introduces a strict mode, which ensures that:\n- no malformed object will be written\n- no object with broken links will be written\n\nThe patch ensures this by delaying the write of all non blob object.\nThese object are written, after all objects they link to are written.\n\nAn error can only result in unreferenced objects.\n\nSigned-off-by: Martin Koegler \n---\n Documentation/git-unpack-objects.txt | 3 +\n builtin-unpack-objects.c | 99 +++++++++++++++++++++++++++++++---\n 2 files changed, 95 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex b79be3f..3697896 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -40,6 +40,9 @@ OPTIONS\n \tand make the best effort to recover as many objects as\n \tpossible.\n \n+--strict::\n+\tDon't write objects with broken content or links.\n+\n \n Author\n ------\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex f18c7e8..3e906e4 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -7,11 +7,13 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"tree.h\"\n+#include \"tree-walk.h\"\n #include \"progress.h\"\n #include \"decorate.h\"\n+#include \"fsck.h\"\n \n-static int dry_run, quiet, recover, has_errors;\n-static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n+static int dry_run, quiet, recover, has_errors, strict;\n+static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] [--strict] < pack-file\";\n \n /* We always read in 4kB chunks. */\n static unsigned char buffer[4096];\n@@ -144,9 +146,58 @@ static void add_delta_to_list(unsigned nr, unsigned const char *base_sha1,\n struct obj_info {\n \toff_t offset;\n \tunsigned char sha1[20];\n+\tstruct object * obj;\n };\n \n+#define FLAG_OPEN (1u<<20)\n+#define FLAG_WRITTEN (1u<<21)\n+\n static struct obj_info *obj_list;\n+unsigned nr_objects;\n+\n+static void write_cached_object(struct object* obj)\n+{\n+\tunsigned char sha1[20];\n+\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\tif (write_sha1_file(obj_buf->buffer, obj_buf->size, typename(obj->type), sha1) < 0)\n+\t\tdie(\"failed to write object %s\", sha1_to_hex(obj->sha1));\n+\tobj->flags |= FLAG_WRITTEN;\n+}\n+\n+static int check_object(struct object* obj, int type, void *data)\n+{\n+\tif (!obj)\n+\t\treturn 0;\n+\t\n+\tif (obj->flags & FLAG_WRITTEN)\n+\t\treturn 1;\n+\t\n+\tif (type != OBJ_ANY && obj->type != type)\n+\t\tdie(\"object type mismatch\");\n+\t\n+\tif (!(obj->flags & FLAG_OPEN)) {\n+\t\tunsigned long size;\n+\t\tint type = sha1_object_info (obj->sha1, &size);\n+\t\tif (type != obj->type || type <= 0)\n+\t\t\tdie(\"object of unexpected type\");\n+\t\tobj->flags |= FLAG_WRITTEN;\n+\t\treturn 1;\n+\t}\n+\t\n+\tif (fsck_object(obj, 1, fsck_error_function))\n+\t die(\"Error in object\");\n+\tif(!fsck_walk(obj, check_object, 0))\n+\t\tdie(\"Error on reachable objects of %s\", sha1_to_hex (obj->sha1));\n+\twrite_cached_object(obj);\n+\treturn 1;\n+}\n+\n+static void write_rest()\n+{\n+\tunsigned i;\n+\tfor (i = 0; i < nr_objects; i++)\n+\t\tcheck_object(obj_list[i].obj, OBJ_ANY, 0);\n+}\n \n static void added_object(unsigned nr, enum object_type type,\n \t\t\t void *data, unsigned long size);\n@@ -154,9 +205,35 @@ static void added_object(unsigned nr, enum object_type type,\n static void write_object(unsigned nr, enum object_type type,\n \t\t\t void *buf, unsigned long size)\n {\n-\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n-\t\tdie(\"failed to write object\");\n \tadded_object(nr, type, buf, size);\n+\tif (!strict) {\n+\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n+\t\t\tdie(\"failed to write object\");\n+\t\tfree(buf);\n+\t\tobj_list[nr].obj = 0;\n+\t} else if (type == OBJ_BLOB) {\n+\t\tstruct blob * blob;\n+\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n+\t\t\tdie(\"failed to write object\");\n+\t\tfree(buf);\n+\n+\t\tblob = lookup_blob (obj_list[nr].sha1);\n+\t\tif (blob)\n+\t\t\tblob->object.flags |= FLAG_WRITTEN;\n+\t\telse\n+\t\t\tdie(\"invalid blob object\");\n+\t\tobj_list[nr].obj = 0;\n+\t} else {\n+\t\tstruct object * obj;\n+\t\tint eaten;\n+\t\thash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);\n+\t\tobj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);\n+\t\tif (!obj)\n+\t\t\tdie (\"invalid %s\", typename(type));\n+\t\tadd_object_buffer (obj, buf, size);\n+\t\tobj->flags |= FLAG_OPEN;\n+\t\tobj_list[nr].obj = obj;\n+\t}\n }\n \n static void resolve_delta(unsigned nr, enum object_type type,\n@@ -173,7 +250,6 @@ static void resolve_delta(unsigned nr, enum object_type type,\n \t\tdie(\"failed to apply delta\");\n \tfree(delta);\n \twrite_object(nr, type, result, result_size);\n-\tfree(result);\n }\n \n static void added_object(unsigned nr, enum object_type type,\n@@ -203,7 +279,8 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n \n \tif (!dry_run && buf)\n \t\twrite_object(nr, type, buf, size);\n-\tfree(buf);\n+\telse if (buf)\n+\t\tfree(buf);\n }\n \n static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n@@ -345,7 +422,8 @@ static void unpack_all(void)\n \tint i;\n \tstruct progress *progress = NULL;\n \tstruct pack_header *hdr = fill(sizeof(struct pack_header));\n-\tunsigned nr_objects = ntohl(hdr->hdr_entries);\n+\n+\tnr_objects = ntohl(hdr->hdr_entries);\n \n \tif (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n@@ -356,6 +434,7 @@ static void unpack_all(void)\n \tif (!quiet)\n \t\tprogress = start_progress(\"Unpacking objects\", nr_objects);\n \tobj_list = xmalloc(nr_objects * sizeof(*obj_list));\n+\tmemset(obj_list, 0, nr_objects * sizeof(*obj_list));\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\n \t\tdisplay_progress(progress, i + 1);\n@@ -391,6 +470,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\trecover = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--strict\")) {\n+\t\t\t\tstrict = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!prefixcmp(arg, \"--pack_header=\")) {\n \t\t\t\tstruct pack_header *hdr;\n \t\t\t\tchar *c;\n@@ -416,6 +499,8 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \tunpack_all();\n \tSHA1_Update(&ctx, buffer, offset);\n \tSHA1_Final(sha1, &ctx);\n+\tif (strict)\n+\t\twrite_rest();\n \tif (hashcmp(fill(20), sha1))\n \t\tdie(\"final sha1 did not match\");\n \tuse(20);\n-- \n1.5.4.g074be\n\n\nFrom e2c4a69caeb7650a2655db864827879a0eea701f Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Fri, 8 Feb 2008 09:16:05 +0100\nSubject: [PATCH 10/12] index-pack: introduce checking mode\n\nAdds strict option, which bails out if the pack would\nintroduces broken object or links in the repository.\n\nSigned-off-by: Martin Koegler \n---\n Documentation/git-index-pack.txt | 3 +\n index-pack.c | 78 +++++++++++++++++++++++++++++++++++--\n 2 files changed, 76 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 72b5d00..a7825b6 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -75,6 +75,9 @@ OPTIONS\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n+--strict::\n+\tDie, if the pack contains broken objects or links.\n+\n \n Note\n ----\ndiff --git a/index-pack.c b/index-pack.c\nindex 9fd6982..bde0249 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -7,9 +7,10 @@\n #include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\n+#include \"fsck.h\"\n \n static const char index_pack_usage[] =\n-\"git-index-pack [-v] [-o ] [{ ---keep | --keep= }] { | --stdin [--fix-thin] [] }\";\n+\"git-index-pack [-v] [-o ] [{ ---keep | --keep= }] [--strict] { | --stdin [--fix-thin] [] }\";\n \n struct object_entry\n {\n@@ -18,6 +19,7 @@ struct object_entry\n \tunsigned int hdr_size;\n \tenum object_type type;\n \tenum object_type real_type;\n+\tstruct object *obj;\n };\n \n union delta_base {\n@@ -31,6 +33,9 @@ union delta_base {\n */\n #define UNION_BASE_SZ\t20\n \n+#define FLAG_OPEN (1u<<20)\n+#define FLAG_WRITTEN (1u<<21)\n+\n struct delta_entry\n {\n \tunion delta_base base;\n@@ -44,6 +49,7 @@ static int nr_deltas;\n static int nr_resolved_deltas;\n \n static int from_stdin;\n+static int strict;\n static int verbose;\n \n static struct progress *progress;\n@@ -56,6 +62,41 @@ static SHA_CTX input_ctx;\n static uint32_t input_crc32;\n static int input_fd, output_fd, pack_fd;\n \n+static int check_object(struct object* obj, int type, void *data)\n+{\n+\tif (!obj)\n+\t\treturn 0;\n+\t\n+\tif (obj->flags & FLAG_WRITTEN)\n+\t\treturn 1;\n+\t\n+\tif (type != OBJ_ANY && obj->type != type)\n+\t\tdie(\"object type mismatch\");\n+\t\n+\tif (!(obj->flags & FLAG_OPEN)) {\n+\t\tunsigned long size;\n+\t\tint type = sha1_object_info (obj->sha1, &size);\n+\t\tif (type != obj->type || type <= 0)\n+\t\t\tdie(\"object of unexpected type\");\n+\t\tobj->flags |= FLAG_WRITTEN;\n+\t\treturn 1;\n+\t}\n+\t\n+\tif (fsck_object(obj, 1, fsck_error_function))\n+\t die(\"Error in object\");\n+\tif(!fsck_walk(obj, check_object, 0))\n+\t\tdie(\"Error on reachable objects of %s\", sha1_to_hex (obj->sha1));\n+\treturn 1;\n+}\n+\n+static void check_objects()\n+{\n+\tunsigned i;\n+\tfor (i = 0; i < nr_objects; i++)\n+\t\tcheck_object(objects[i].obj, OBJ_ANY, 0);\n+}\n+\n+\n /* Discard current buffer used content. */\n static void flush(void)\n {\n@@ -325,8 +366,8 @@ static int find_delta_children(const union delta_base *base,\n \treturn 0;\n }\n \n-static void sha1_object(const void *data, unsigned long size,\n-\t\t\tenum object_type type, unsigned char *sha1)\n+static struct object* sha1_object(const void *data, unsigned long size,\n+\t\t\t\t enum object_type type, unsigned char *sha1)\n {\n \thash_sha1_file(data, size, typename(type), sha1);\n \tif (has_sha1_file(sha1)) {\n@@ -341,6 +382,29 @@ static void sha1_object(const void *data, unsigned long size,\n \t\t\tdie(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n \t\tfree(has_data);\n \t}\n+\tif (strict) {\n+\t\tif (type == OBJ_BLOB) {\n+\t\t\tstruct blob * blob = lookup_blob(sha1);\n+\t\t\tif (blob)\n+\t\t\t\tblob->object.flags |= FLAG_WRITTEN;\n+\t\t\telse\n+\t\t\t\tdie(\"invalid blob object %s\", sha1_to_hex(sha1));\n+\t\t\treturn (struct object*)blob;\n+\t\t} else {\n+\t\t\tstruct object * obj;\n+\t\t\tint eaten;\n+\t\t\tvoid *buf = xmemdupz(data, size);\n+\t\t\t\n+\t\t\tobj = parse_object_buffer(sha1, type, size, buf, &eaten);\n+\t\t\tif (!obj)\n+\t\t\t\tdie (\"invalid %s\", typename(type));\n+\t\t\tif(!eaten)\n+\t\t\t\tfree(buf);\n+\t\t\tobj->flags |= FLAG_OPEN;\n+\t\t\treturn obj;\n+\t\t}\n+\t}\n+\treturn NULL;\n }\n \n static void resolve_delta(struct object_entry *delta_obj, void *base_data,\n@@ -361,7 +425,7 @@ static void resolve_delta(struct object_entry *delta_obj, void *base_data,\n \tfree(delta_data);\n \tif (!result)\n \t\tbad_object(delta_obj->idx.offset, \"failed to apply delta\");\n-\tsha1_object(result, result_size, type, delta_obj->idx.sha1);\n+\tdelta_obj->obj = sha1_object(result, result_size, type, delta_obj->idx.sha1);\n \tnr_resolved_deltas++;\n \n \thashcpy(delta_base.sha1, delta_obj->idx.sha1);\n@@ -420,7 +484,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\tdelta->obj_no = i;\n \t\t\tdelta++;\n \t\t} else\n-\t\t\tsha1_object(data, obj->size, obj->type, obj->idx.sha1);\n+\t\t\tobj->obj = sha1_object(data, obj->size, obj->type, obj->idx.sha1);\n \t\tfree(data);\n \t\tdisplay_progress(progress, i+1);\n \t}\n@@ -714,6 +778,8 @@ int main(int argc, char **argv)\n \t\t\t\tfrom_stdin = 1;\n \t\t\t} else if (!strcmp(arg, \"--fix-thin\")) {\n \t\t\t\tfix_thin_pack = 1;\n+\t\t\t} else if (!strcmp(arg, \"--strict\")) {\n+\t\t\t\tstrict = 1;\n \t\t\t} else if (!strcmp(arg, \"--keep\")) {\n \t\t\t\tkeep_msg = \"\";\n \t\t\t} else if (!prefixcmp(arg, \"--keep=\")) {\n@@ -812,6 +878,8 @@ int main(int argc, char **argv)\n \t\t\t nr_deltas - nr_resolved_deltas);\n \t}\n \tfree(deltas);\n+\tif (strict)\n+\t\tcheck_objects();\n \n \tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n \tfor (i = 0; i < nr_objects; i++)\n-- \n1.5.4.g074be\n\n\nFrom a992fadf4014830315d7b5053dc633c2c566ae44 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Sat, 9 Feb 2008 18:17:56 +0100\nSubject: [PATCH 11/12] receive-pack: use strict mode for unpacking objects\n\nSigned-off-by: Martin Koegler \n---\n Documentation/config.txt | 6 ++++++\n receive-pack.c | 36 +++++++++++++++++++++++-------------\n 2 files changed, 29 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 530a6a8..d5b7698 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -909,6 +909,12 @@ imap::\n \tThe configuration variables in the 'imap' section are described\n \tin linkgit:git-imap-send[1].\n \n+receive.fsckObjects::\n+\tIf it is set to true, git-receive-pack will check all received \n+\tobjects. It will abort in the case of a malformed object or a \n+\tbroken link. The result of an abort are only dangling objects.\n+\tThe default value is false.\n+\n receive.unpackLimit::\n \tIf the number of objects received in a push is below this\n \tlimit then the objects will be unpacked into loose object\ndiff --git a/receive-pack.c b/receive-pack.c\nindex 3267495..7136f35 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -10,6 +10,7 @@\n static const char receive_pack_usage[] = \"git-receive-pack \";\n \n static int deny_non_fast_forwards = 0;\n+static int receive_fsck_objects = 0;\n static int receive_unpack_limit = -1;\n static int transfer_unpack_limit = -1;\n static int unpack_limit = 100;\n@@ -35,6 +36,11 @@ static int receive_pack_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.fsckobjects\") == 0) {\n+\t\treceive_fsck_objects = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value);\n }\n \n@@ -367,11 +373,13 @@ static const char *unpack(void)\n \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n-\t\tint code;\n-\t\tconst char *unpacker[3];\n-\t\tunpacker[0] = \"unpack-objects\";\n-\t\tunpacker[1] = hdr_arg;\n-\t\tunpacker[2] = NULL;\n+\t\tint code, i = 0;\n+\t\tconst char *unpacker[4];\n+\t\tunpacker[i++] = \"unpack-objects\";\n+\t\tif (receive_fsck_objects)\n+\t\t\tunpacker[i++] = \"--strict\";\n+\t\tunpacker[i++] = hdr_arg;\n+\t\tunpacker[i++] = NULL;\n \t\tcode = run_command_v_opt(unpacker, RUN_GIT_CMD);\n \t\tswitch (code) {\n \t\tcase 0:\n@@ -392,8 +400,8 @@ static const char *unpack(void)\n \t\t\treturn \"unpacker exited with error code\";\n \t\t}\n \t} else {\n-\t\tconst char *keeper[6];\n-\t\tint s, status;\n+\t\tconst char *keeper[7];\n+\t\tint s, status, i = 0;\n \t\tchar keep_arg[256];\n \t\tstruct child_process ip;\n \n@@ -401,12 +409,14 @@ static const char *unpack(void)\n \t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n \t\t\tstrcpy(keep_arg + s, \"localhost\");\n \n-\t\tkeeper[0] = \"index-pack\";\n-\t\tkeeper[1] = \"--stdin\";\n-\t\tkeeper[2] = \"--fix-thin\";\n-\t\tkeeper[3] = hdr_arg;\n-\t\tkeeper[4] = keep_arg;\n-\t\tkeeper[5] = NULL;\n+\t\tkeeper[i++] = \"index-pack\";\n+\t\tkeeper[i++] = \"--stdin\";\n+\t\tif (receive_fsck_objects)\n+\t\t\tkeeper[i++] = \"--strict\";\n+\t\tkeeper[i++] = \"--fix-thin\";\n+\t\tkeeper[i++] = hdr_arg;\n+\t\tkeeper[i++] = keep_arg;\n+\t\tkeeper[i++] = NULL;\n \t\tmemset(&ip, 0, sizeof(ip));\n \t\tip.argv = keeper;\n \t\tip.out = -1;\n-- \n1.5.4.g074be\n\n\nFrom c39a307f2f6f551d967cbc9fb502de7cad8beae6 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Sat, 9 Feb 2008 18:18:41 +0100\nSubject: [PATCH 12/12] Remove unused object-ref code\n\nSigned-off-by: Martin Koegler \n---\n Makefile | 2 +-\n builtin-fetch-pack.c | 1 -\n builtin-pack-objects.c | 1 -\n builtin-rev-list.c | 1 -\n commit.c | 11 ------\n object-refs.c | 87 ------------------------------------------------\n object.h | 8 ----\n tag.c | 6 ---\n tree.c | 48 --------------------------\n upload-pack.c | 1 -\n walker.c | 1 -\n 11 files changed, 1 insertions(+), 166 deletions(-)\n delete mode 100644 object-refs.c\n\ndiff --git a/Makefile b/Makefile\nindex 0921484..268b8dd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -316,7 +316,7 @@ LIB_OBJS = \\\n \tpatch-ids.o \\\n \tobject.o pack-check.o pack-write.o patch-delta.o path.o pkt-line.o \\\n \tsideband.o reachable.o reflog-walk.o \\\n-\tquote.o read-cache.o refs.o run-command.o dir.o object-refs.o \\\n+\tquote.o read-cache.o refs.o run-command.o dir.o \\\n \tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o \\\n \ttag.o tree.o usage.o config.o environment.o ctype.o copy.o \\\n \trevision.o pager.o tree-walk.o xdiff-interface.o \\\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex f401352..25f1915 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -386,7 +386,6 @@ static int everything_local(struct ref **refs, int nr_match, char **match)\n \tint retval;\n \tunsigned long cutoff = 0;\n \n-\ttrack_object_refs = 0;\n \tsave_commit_buffer = 0;\n \n \tfor (ref = *refs; ref; ref = ref->next) {\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex d3efeff..6f8f388 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2009,7 +2009,6 @@ static void get_object_list(int ac, const char **av)\n \n \tinit_revisions(&revs, NULL);\n \tsave_commit_buffer = 0;\n-\ttrack_object_refs = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex de80158..14e86ce 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -605,7 +605,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tusage(rev_list_usage);\n \n \tsave_commit_buffer = revs.verbose_header || revs.grep_filter;\n-\ttrack_object_refs = 0;\n \tif (bisect_list)\n \t\trevs.limited = 1;\n \ndiff --git a/commit.c b/commit.c\nindex 8b8fb04..df4d331 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -290,17 +290,6 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t}\n \titem->date = parse_commit_date(bufptr, tail);\n \n-\tif (track_object_refs) {\n-\t\tunsigned i = 0;\n-\t\tstruct commit_list *p;\n-\t\tstruct object_refs *refs = alloc_object_refs(n_refs);\n-\t\tif (item->tree)\n-\t\t\trefs->ref[i++] = &item->tree->object;\n-\t\tfor (p = item->parents; p; p = p->next)\n-\t\t\trefs->ref[i++] = &p->item->object;\n-\t\tset_object_refs(&item->object, refs);\n-\t}\n-\n \treturn 0;\n }\n \ndiff --git a/object-refs.c b/object-refs.c\ndeleted file mode 100644\nindex 5345671..0000000\n--- a/object-refs.c\n+++ /dev/null\n@@ -1,87 +0,0 @@\n-#include \"cache.h\"\n-#include \"object.h\"\n-#include \"decorate.h\"\n-\n-int track_object_refs = 0;\n-\n-static struct decoration ref_decorate;\n-\n-struct object_refs *lookup_object_refs(struct object *base)\n-{\n-\treturn lookup_decoration(&ref_decorate, base);\n-}\n-\n-static void add_object_refs(struct object *obj, struct object_refs *refs)\n-{\n-\tif (add_decoration(&ref_decorate, obj, refs))\n-\t\tdie(\"object %s tried to add refs twice!\", sha1_to_hex(obj->sha1));\n-}\n-\n-struct object_refs *alloc_object_refs(unsigned count)\n-{\n-\tstruct object_refs *refs;\n-\tsize_t size = sizeof(*refs) + count*sizeof(struct object *);\n-\n-\trefs = xcalloc(1, size);\n-\trefs->count = count;\n-\treturn refs;\n-}\n-\n-static int compare_object_pointers(const void *a, const void *b)\n-{\n-\tconst struct object * const *pa = a;\n-\tconst struct object * const *pb = b;\n-\tif (*pa == *pb)\n-\t\treturn 0;\n-\telse if (*pa < *pb)\n-\t\treturn -1;\n-\telse\n-\t\treturn 1;\n-}\n-\n-void set_object_refs(struct object *obj, struct object_refs *refs)\n-{\n-\tunsigned int i, j;\n-\n-\t/* Do not install empty list of references */\n-\tif (refs->count < 1) {\n-\t\tfree(refs);\n-\t\treturn;\n-\t}\n-\n-\t/* Sort the list and filter out duplicates */\n-\tqsort(refs->ref, refs->count, sizeof(refs->ref[0]),\n-\t compare_object_pointers);\n-\tfor (i = j = 1; i < refs->count; i++) {\n-\t\tif (refs->ref[i] != refs->ref[i - 1])\n-\t\t\trefs->ref[j++] = refs->ref[i];\n-\t}\n-\tif (j < refs->count) {\n-\t\t/* Duplicates were found - reallocate list */\n-\t\tsize_t size = sizeof(*refs) + j*sizeof(struct object *);\n-\t\trefs->count = j;\n-\t\trefs = xrealloc(refs, size);\n-\t}\n-\n-\tfor (i = 0; i < refs->count; i++)\n-\t\trefs->ref[i]->used = 1;\n-\tadd_object_refs(obj, refs);\n-}\n-\n-void mark_reachable(struct object *obj, unsigned int mask)\n-{\n-\tconst struct object_refs *refs;\n-\n-\tif (!track_object_refs)\n-\t\tdie(\"cannot do reachability with object refs turned off\");\n-\t/* If we've been here already, don't bother */\n-\tif (obj->flags & mask)\n-\t\treturn;\n-\tobj->flags |= mask;\n-\trefs = lookup_object_refs(obj);\n-\tif (refs) {\n-\t\tunsigned i;\n-\t\tfor (i = 0; i < refs->count; i++)\n-\t\t\tmark_reachable(refs->ref[i], mask);\n-\t}\n-}\ndiff --git a/object.h b/object.h\nindex 397bbfa..036bd66 100644\n--- a/object.h\n+++ b/object.h\n@@ -35,14 +35,11 @@ struct object {\n \tunsigned char sha1[20];\n };\n \n-extern int track_object_refs;\n-\n extern const char *typename(unsigned int type);\n extern int type_from_string(const char *str);\n \n extern unsigned int get_max_object_index(void);\n extern struct object *get_indexed_object(unsigned int);\n-extern struct object_refs *lookup_object_refs(struct object *);\n \n /** Internal only **/\n struct object *lookup_object(const unsigned char *sha1);\n@@ -61,11 +58,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n /** Returns the object, with potentially excess memory allocated. **/\n struct object *lookup_unknown_object(const unsigned char *sha1);\n \n-struct object_refs *alloc_object_refs(unsigned count);\n-void set_object_refs(struct object *obj, struct object_refs *refs);\n-\n-void mark_reachable(struct object *obj, unsigned int mask);\n-\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p);\n \ndiff --git a/tag.c b/tag.c\nindex 38bf913..d19f56d 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -84,12 +84,6 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t\titem->tagged = NULL;\n \t}\n \n-\tif (item->tagged && track_object_refs) {\n-\t\tstruct object_refs *refs = alloc_object_refs(1);\n-\t\trefs->ref[0] = item->tagged;\n-\t\tset_object_refs(&item->object, refs);\n-\t}\n-\n \treturn 0;\n }\n \ndiff --git a/tree.c b/tree.c\nindex 87708ef..4b1825c 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -202,52 +202,6 @@ struct tree *lookup_tree(const unsigned char *sha1)\n \treturn (struct tree *) obj;\n }\n \n-/*\n- * NOTE! Tree refs to external git repositories\n- * (ie gitlinks) do not count as real references.\n- *\n- * You don't have to have those repositories\n- * available at all, much less have the objects\n- * accessible from the current repository.\n- */\n-static void track_tree_refs(struct tree *item)\n-{\n-\tint n_refs = 0, i;\n-\tstruct object_refs *refs;\n-\tstruct tree_desc desc;\n-\tstruct name_entry entry;\n-\n-\t/* Count how many entries there are.. */\n-\tinit_tree_desc(&desc, item->buffer, item->size);\n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tif (S_ISGITLINK(entry.mode))\n-\t\t\tcontinue;\n-\t\tn_refs++;\n-\t}\n-\n-\t/* Allocate object refs and walk it again.. */\n-\ti = 0;\n-\trefs = alloc_object_refs(n_refs);\n-\tinit_tree_desc(&desc, item->buffer, item->size);\n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tstruct object *obj;\n-\n-\t\tif (S_ISGITLINK(entry.mode))\n-\t\t\tcontinue;\n-\t\tif (S_ISDIR(entry.mode))\n-\t\t\tobj = &lookup_tree(entry.sha1)->object;\n-\t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode))\n-\t\t\tobj = &lookup_blob(entry.sha1)->object;\n-\t\telse {\n-\t\t\twarning(\"in tree %s: entry %s has bad mode %.6o\\n\",\n-\t\t\t sha1_to_hex(item->object.sha1), entry.path, entry.mode);\n-\t\t\tobj = lookup_unknown_object(entry.sha1);\n-\t\t}\n-\t\trefs->ref[i++] = obj;\n-\t}\n-\tset_object_refs(&item->object, refs);\n-}\n-\n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n {\n \tif (item->object.parsed)\n@@ -256,8 +210,6 @@ int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n \titem->buffer = buffer;\n \titem->size = size;\n \n-\tif (track_object_refs)\n-\t\ttrack_tree_refs(item);\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7e04311..b2828ab 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -392,7 +392,6 @@ static int get_common_commits(void)\n \tchar hex[41], last_hex[41];\n \tint len;\n \n-\ttrack_object_refs = 0;\n \tsave_commit_buffer = 0;\n \n \tfor(;;) {\ndiff --git a/walker.c b/walker.c\nindex adc3e80..c10eca8 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -256,7 +256,6 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tint i;\n \n \tsave_commit_buffer = 0;\n-\ttrack_object_refs = 0;\n \n \tfor (i = 0; i < targets; i++) {\n \t\tif (!write_ref || !write_ref[i])\n-- \n1.5.4.g074be\n"},{"id":"68298","messageId":"7vmyq8cqfn.fsf@gitster.siamese.dyndns.org","threadId":"12011","inReplyTo":"20080210175812.GB12162@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-11T00:00:44Z","receivedAt":"2008-02-11T00:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n\n> So I tried to fix the problem in another way: Instead of coping with \n> corrupt objects, stop them from entering the repository.\n\nGood intention. Very nice.\n\n> * add --strict option to unpack-objects (patch 1,8,9)\n> * add --strict option to index-pack (patch 8,10)\n>\n> Same as for unpack-objects, but without writting objects.\n>\n> * add config option for receive pack to enable checking (patch 11)\n\nIf this patch is any good, I strongly suspect it should not be\njust the default but should always be on. IOW no config is\nnecessary. That would make the series a bit simpler, I guess.\n\n> From 76e86fe55345e633c910d6b8fe166e27c23c5aaf Mon Sep 17 00:00:00 2001\n> From: Martin Koegler \n> Date: Fri, 8 Feb 2008 08:51:38 +0100\n> Subject: [PATCH 01/12] unpack-object: cache for non written objects\n>\n> Preventing objects with broken links entering the repository\n> means, that write of some objects must be delayed.\n>\n> This patch adds a cache to keep the object data in memory. The delta\n> resolving code must also search in the cache.\n\nI have to wonder what the memory pressure in real-life usage\nwill be like.\n\nWhen an object is proven to be good, we should be able to free\nits buffer after writing it out, but would that be a good enough\noptimization we can make later on this code to keep its memory\nconsumption manageable?\n\n> diff --git a/fsck.c b/fsck.c\n> new file mode 100644\n> index 0000000..089f775\n> --- /dev/null\n> +++ b/fsck.c\n> @@ -0,0 +1,84 @@\n> +#include \"cache.h\"\n> +#include \"object.h\"\n> +#include \"blob.h\"\n> +#include \"tree.h\"\n> +#include \"tree-walk.h\"\n> +#include \"commit.h\"\n> +#include \"tag.h\"\n> +#include \"fsck.h\"\n> +\n> +static int fsck_walk_tree(struct tree* tree, fsck_walk_func walk, void* data)\n> +{\n> +\tstruct tree_desc desc;\n> +\tstruct name_entry entry;\n> +\n> +\tif(parse_tree(tree))\n> +\t\treturn -1;\n\nI noticed many coding style issues but I won't be mentioning\nthem in this response.\n\nIt's a bit hard to see how these new set of functions relate to\nthe original code in this patch series, because you add the new\nthings that are initially not used anywhere independently, start\nreferring to them in a separate patch and then remove the old\nrelated functions that are now unused. This style makes\nreviewing easier and harder at the same time...\n\n> From 80b22c3f2c3e13c207790a49646020c55b34bba7 Mon Sep 17 00:00:00 2001\n> From: Martin Koegler \n> Date: Fri, 8 Feb 2008 09:01:50 +0100\n> Subject: [PATCH 03/12] fsck: move mark-reachable to fsck_walk\n>\n> Signed-off-by: Martin Koegler \n> ---\n> builtin-fsck.c | 34 ++++++++++++++++++++++++----------\n> 1 files changed, 24 insertions(+), 10 deletions(-)\n> ...\n> +static int mark_object(struct object* obj, int type, void* data)\n> +{\n> +\tif (!obj)\n> +\t\treturn 0;\n> +\tif (obj->flags & REACHABLE)\n> +\t\treturn 0;\n> +\tobj->flags |= REACHABLE;\n> +\tif (!obj->parsed)\n> +\t\treturn 0;\n> +\treturn fsck_walk(obj, mark_object, data);\n> +}\n\nHmm. The return value 0 means Ok and negative is error? The\nreason we can say success if obj is NULL or it is not parsed yet\nis because...?\n\n> @@ -326,8 +344,6 @@ static int fsck_tree(struct tree *item)\n> \t\to_name = name;\n> \t\to_sha1 = sha1;\n> \t}\n> -\tfree(item->buffer);\n> -\titem->buffer = NULL;\n\nHmm. The reason you still need the buffer after you checked the\ncontents of the tree in the loop is because you haven't actually\nchecked the referents are Ok. But I do not see a corresponding\nfree that releases this memory after you are actually done with\nthe verification with fsck_walk() yet, so we leak this in the\nmeantime?\n\n> @@ -375,8 +391,6 @@ static int fsck_commit(struct commit *commit)\n> \t}\n> \tif (memcmp(buffer, \"author \", 7))\n> \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n> -\tfree(commit->buffer);\n> -\tcommit->buffer = NULL;\n> \tif (!commit->tree)\n> \t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n> \tif (!commit->parents && show_root)\n\nLikewise.\n\n> From ce43251ef71962ff64fe138f1295c405ef6aaf65 Mon Sep 17 00:00:00 2001\n> From: Martin Koegler \n> Date: Fri, 8 Feb 2008 09:04:08 +0100\n> Subject: [PATCH 04/12] fsck: move reachable object check to fsck_walk\n>\n> It handles NULL pointers in object references without crashing.\n>\n> Signed-off-by: Martin Koegler \n> ---\n> builtin-fsck.c | 49 +++++++++++++++++++++++++++++--------------------\n> 1 files changed, 29 insertions(+), 20 deletions(-)\n>\n> diff --git a/builtin-fsck.c b/builtin-fsck.c\n> index 49e96ff..2c1e10f 100644\n> --- a/builtin-fsck.c\n> +++ b/builtin-fsck.c\n> @@ -81,13 +81,39 @@ static int objwarning(struct object *obj, const char *err, ...)\n> \treturn -1;\n> }\n> \n> +static int check_reachable_object_childs(struct object *obj, int type, void *data)\n> +{\n> +\tstruct object *parent = data;\n> +\tif (!obj) {\n> +\t\tprintf(\"broken link from %7s %s\\n\",\n> +\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n> +\t\tprintf(\"broken link from %7s %s\\n\",\n> +\t\t\t (type==OBJ_ANY?\"unknown\":typename(type)), \"unknown\");\n\nHmm? I am not sure what this part is reporting...\n\n> From ee11f771be1ef1c29725cb56ab3eb8dfe61ca25a Mon Sep 17 00:00:00 2001\n> From: Martin Koegler \n> Date: Fri, 8 Feb 2008 09:07:33 +0100\n> Subject: [PATCH 06/12] create a common object checker code out of fsck\n>\n> The function provides a callback for reporting errors.\n\nThe same \"add unused new stuff independently, later use it and\nthen finally remove now unused old stuff\" pattern is here. I am\nneutral to that patch style but it is a bit harder to see what\nis going on.\n\nMost of the changes seem to be straight and sane copy-and-paste though.\n\n> From a8db4e754e717bac0b2462333d4145eac3452099 Mon Sep 17 00:00:00 2001\n> From: Martin Koegler \n> Date: Fri, 8 Feb 2008 09:14:14 +0100\n> Subject: [PATCH 09/12] unpack-objects: prevent writing of inconsistent objects\n>\n> This patch introduces a strict mode, which ensures that:\n> - no malformed object will be written\n> - no object with broken links will be written\n>\n> The patch ensures this by delaying the write of all non blob object.\n> These object are written, after all objects they link to are written.\n>\n> An error can only result in unreferenced objects.\n\n> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\n> index f18c7e8..3e906e4 100644\n> --- a/builtin-unpack-objects.c\n> +++ b/builtin-unpack-objects.c\n> @@ -173,7 +250,6 @@ static void resolve_delta(unsigned nr, enum object_type type,\n> \t\tdie(\"failed to apply delta\");\n> \tfree(delta);\n> \twrite_object(nr, type, result, result_size);\n> -\tfree(result);\n> }\n\nAnd this is freed later elsewhere?\n\n> @@ -203,7 +279,8 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n> \n> \tif (!dry_run && buf)\n> \t\twrite_object(nr, type, buf, size);\n> -\tfree(buf);\n> +\telse if (buf)\n> +\t\tfree(buf);\n> }\n\nYou can always free NULL without checking.\n\n> @@ -356,6 +434,7 @@ static void unpack_all(void)\n> \tif (!quiet)\n> \t\tprogress = start_progress(\"Unpacking objects\", nr_objects);\n> \tobj_list = xmalloc(nr_objects * sizeof(*obj_list));\n> +\tmemset(obj_list, 0, nr_objects * sizeof(*obj_list));\n\nHmm, is this a fix to the 'master' independent from all the rest\nof your patches, or a new requirement?\n"},{"id":"68299","messageId":"alpine.LFD.1.00.0802101929310.2732@xanadu.home","threadId":"12011","inReplyTo":"7vmyq8cqfn.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-11T00:33:34Z","receivedAt":"2008-02-11T00:33:34Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 10 Feb 2008, Junio C Hamano wrote:\n\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> \n> > This patch adds a cache to keep the object data in memory. The delta\n> > resolving code must also search in the cache.\n> \n> I have to wonder what the memory pressure in real-life usage\n> will be like.\n\nFWIW, I don't like this idea.\n\nI'm struggling to find ways to improve performances of \npack-objects/index-pack with those large repositories that are becoming \nmore common (i.e. GCC, OOO, Mozilla, etc.) Anything that increase \nmemory usage isn't very welcome IMHO.\n\n\nNicolas\n"},{"id":"68433","messageId":"20080211195623.GA21878@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802101929310.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-11T19:56:23Z","receivedAt":"2008-02-11T19:56:23Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 10, 2008 at 07:33:34PM -0500, Nicolas Pitre wrote:\n> On Sun, 10 Feb 2008, Junio C Hamano wrote:\n> \n> > mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > \n> > > This patch adds a cache to keep the object data in memory. The delta\n> > > resolving code must also search in the cache.\n> > \n> > I have to wonder what the memory pressure in real-life usage\n> > will be like.\n\n> FWIW, I don't like this idea.\n>\n> I'm struggling to find ways to improve performances of \n> pack-objects/index-pack with those large repositories that are becoming \n> more common (i.e. GCC, OOO, Mozilla, etc.) Anything that increase \n> memory usage isn't very welcome IMHO.\n\nMaybe I have missed something, but all repack problems reported on the\ngit mailing list happen durring the deltifing phase. The problematic\nfiles are mostly bigger blobs. I'm aware of these problems, so my\npatch does not keep any blobs in memory.\n\nAs we are talking about memory, let's ignore unpack-objects, which is\nused for small packs. Lets compare the memory usage of index-pack to\npack-objects:\n\nIf it is disabled (no --strict passed), only a (unused) pointer for\neach object in the received pack file is additionally allocated.\n\nOn i386, struct object_entry is 84 bytes in pack-objects, but only 52\nin index-pack. Both programs keep a struct object_entry for each\nobject during the runtime in memory. So in this case, index-pack uses\nless memory than pack-objects\n\nIf the --strict option is passed, more memory is used:\n\n* Again, we add one pointer to struct object_entry. object_entry is\n still smaller.(52<84 bytes).\n\n* index-pack allocates a struct blob/tree/commit/tag for each object in the pack.\n\n pack-objects also allocates only struct object in the best case\n (reading from pack file), otherwise a struct\n blob/tree/commit/tag. This objects are kept during the runtime of\n pack-objects in memory.\n\n So depending of the parameters of pack-objects, index-pack uses\n additionally up to 24 bytes per object, but struct object_entry is 32\n bytes smaller.\n\n* index-pack allocates a struct blob/tree/commit/tag for each link to a object outside the pack.\n\n I don't know the code of pack-objects enough to say something to\n this point.\n\n* index-pack keeps the data for each tag/tree/commit in the pack in memory\n\n In the next version, I don't need to keep the tag/commit data in\n memory. Tree data could be reconstructed from the written pack,\n but I'm not sure, if the additional code (resolving deltas again),\n would justify the additional memory usage.\n\nSo my conclusion is, that the memory usage of index-pack with --strict\nshould not be too worse compared to pack-objects.\n\nPlease remember, that --strict is used for pushing data.\n\nmfg Martin Kögler\n"},{"id":"68437","messageId":"alpine.LFD.1.00.0802111513360.2732@xanadu.home","threadId":"12011","inReplyTo":"20080211195623.GA21878@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-11T20:41:06Z","receivedAt":"2008-02-11T20:41:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 11 Feb 2008, Martin Koegler wrote:\n\n> On Sun, Feb 10, 2008 at 07:33:34PM -0500, Nicolas Pitre wrote:\n> > On Sun, 10 Feb 2008, Junio C Hamano wrote:\n> > \n> > > mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > > \n> > > > This patch adds a cache to keep the object data in memory. The delta\n> > > > resolving code must also search in the cache.\n> > > \n> > > I have to wonder what the memory pressure in real-life usage\n> > > will be like.\n> \n> > FWIW, I don't like this idea.\n> >\n> > I'm struggling to find ways to improve performances of \n> > pack-objects/index-pack with those large repositories that are becoming \n> > more common (i.e. GCC, OOO, Mozilla, etc.) Anything that increase \n> > memory usage isn't very welcome IMHO.\n> \n> Maybe I have missed something, but all repack problems reported on the\n> git mailing list happen durring the deltifing phase. The problematic\n> files are mostly bigger blobs. I'm aware of these problems, so my\n> patch does not keep any blobs in memory.\n\nThis is not only repack, which is something that few people should run \nanyway.\n\nIt is also index-pack which takes an awful amount of time on the OOO \nrepo (much less than the repack but still), and that's something that is \nvisible to anyone cloning it.\n\nSo the comparison with pack-objects is useless. In absolute terms, \nindex-pack has to inprove, not regress.\n\n> So my conclusion is, that the memory usage of index-pack with --strict\n> should not be too worse compared to pack-objects.\n\nWell, the comparison is flawed anyway. Agressively repacking the OOO \nrepo is reported to take around 14 GB of RAM if not bounded, whereas \nindex-pack remained around 300MB. But the buffer cache managed by the \nkernel is also a big performance factor and anything that requires more \nmemory in user space will affect that cache.\n\nThen remember that this repo has 2411764 objects.\n\n> Please remember, that --strict is used for pushing data.\n\nAnd what exactly does it provide in terms of safetiness that index-pack \ndoesn't already do? I'm not sure I fully understood the goal here.\n\n\nNicolas\n"},{"id":"68449","messageId":"20080211215806.GA24971@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802111513360.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-11T21:58:06Z","receivedAt":"2008-02-11T21:58:06Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Mon, Feb 11, 2008 at 03:41:06PM -0500, Nicolas Pitre wrote:\n> On Mon, 11 Feb 2008, Martin Koegler wrote:\n> > Please remember, that --strict is used for pushing data.\n> \n> And what exactly does it provide in terms of safetiness that index-pack \n> doesn't already do? I'm not sure I fully understood the goal here.\n\nThe patch is about verifing the content of objects. My intented use is\nfor running a (restricted) server, which allows pushing via git-daemon\n(or git-shell) from (untrusted) users. It can be also used to catch\ncorrupt objects, before they leave your repository, but this is for me\nonly a side effect.\n\nreceive-pack accepts any crap (anything, which you can pipe into\ngit-hash-object with -t parameter of your choice), if the pack file\nformat is correct.\n\nBut lots of code in git assums that the object content is welformd.\n\nHaving such objects somewhere reachable in your repository will\ndisturb cloning (In the best case you get a error messages, in the\nworst a child process of upload-pack segfaults), gitweb, ... . To fix\nit, you will need a person with native access to the repository in the\nworst case. \n\n> > Maybe I have missed something, but all repack problems reported on the\n> > git mailing list happen durring the deltifing phase. The problematic\n> > files are mostly bigger blobs. I'm aware of these problems, so my\n> > patch does not keep any blobs in memory.\n> \n> This is not only repack, which is something that few people should run \n> anyway.\n> \n> It is also index-pack which takes an awful amount of time on the OOO \n> repo (much less than the repack but still), and that's something that is \n> visible to anyone cloning it.\n> \n> So the comparison with pack-objects is useless. In absolute terms, \n> index-pack has to inprove, not regress.\n\nThe patch does not change the cloning/fetching behaviour:\n@@ -18,6 +19,7 @@ struct object_entry\n unsigned int hdr_size;\n enum object_type type;\n enum object_type real_type;\n+ struct object *obj;\n };\n\n union delta_base {\n@@ -44,6 +49,7 @@ static int nr_deltas;\n static int nr_resolved_deltas;\n\n static int from_stdin;\n+static int strict;\n static int verbose;\n\n static struct progress *progress;\n@@ -325,8 +366,8 @@ static int find_delta_children(const union delta_base *base,\n return 0;\n }\n\n-static void sha1_object(const void *data, unsigned long size,\n- enum object_type type, unsigned char *sha1)\n+static struct object* sha1_object(const void *data, unsigned long size,\n+ enum object_type type, unsigned char *sha1)\n {\n hash_sha1_file(data, size, typename(type), sha1);\n if (has_sha1_file(sha1)) {\n@@ -341,6 +382,29 @@ static void sha1_object(const void *data, unsigned long size,\n die(\"SHA1 COLLISION FOUND WITH %s !\", sha1_to_hex(sha1));\n free(has_data);\n }\n+ if (strict) {\n[..]\n+ return NULL;\n }\n\n static void resolve_delta(struct object_entry *delta_obj, void *base_data,\n@@ -361,7 +425,7 @@ static void resolve_delta(struct object_entry *delta_obj, void *base_data,\n free(delta_data);\n if (!result)\n bad_object(delta_obj->idx.offset, \"failed to apply delta\");\n- sha1_object(result, result_size, type, delta_obj->idx.sha1);\n+ delta_obj->obj = sha1_object(result, result_size, type, delta_obj->idx.sha1);\n nr_resolved_deltas++;\n\n hashcpy(delta_base.sha1, delta_obj->idx.sha1);\n@@ -420,7 +484,7 @@ static void parse_pack_objects(unsigned char *sha1)\n delta->obj_no = i;\n delta++;\n } else\n- sha1_object(data, obj->size, obj->type, obj->idx.sha1);\n+ obj->obj = sha1_object(data, obj->size, obj->type, obj->idx.sha1);\n free(data);\n display_progress(progress, i+1);\n }\n@@ -714,6 +778,8 @@ int main(int argc, char **argv)\n from_stdin = 1;\n } else if (!strcmp(arg, \"--fix-thin\")) {\n fix_thin_pack = 1;\n+ } else if (!strcmp(arg, \"--strict\")) {\n+ strict = 1;\n } else if (!strcmp(arg, \"--keep\")) {\n keep_msg = \"\";\n } else if (!prefixcmp(arg, \"--keep=\")) {\n@@ -812,6 +878,8 @@ int main(int argc, char **argv)\n nr_deltas - nr_resolved_deltas);\n }\n free(deltas);\n+ if (strict)\n+ check_objects();\n\n idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n for (i = 0; i < nr_objects; i++)\n\nDo you see any problem, if int strict is zero?\n\nFor pushing, you must distinguish two case:\n\n* pushing some changes/new branches\n You will have a maximum of a few thousands of objects.\n\n* pushing the whole content into an empty repository\n\n This is not an every day operation. \n\n I implemented a config option to disable the checking. It can be\n used safe time/memory in such cases.\n \n To make this working for OOO repository, my patch will probable need\n more tuning.\n\n> > So my conclusion is, that the memory usage of index-pack with --strict\n> > should not be too worse compared to pack-objects.\n> \n> Well, the comparison is flawed anyway. Agressively repacking the OOO \n> repo is reported to take around 14 GB of RAM if not bounded, whereas \n> index-pack remained around 300MB. But the buffer cache managed by the \n> kernel is also a big performance factor and anything that requires more \n> memory in user space will affect that cache.\n\nThe 14 GB is caused by the memory used in the deltifing phase. I\ncompared it to the counting object phase.\n\n> Then remember that this repo has 2411764 objects.\n\nHow many blobs, tags, trees and commits? What is the uncompressed size\nof all tags? What is the uncompressed size of all trees? What is the\nuncompressed size of all commits?\n\nThis information would give me a clue, what bad effects my approach\nwould have.\n\nmfg Martin Kögler\n"},{"id":"68487","messageId":"20080212072030.GA20857@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"7vmyq8cqfn.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-12T07:20:30Z","receivedAt":"2008-02-12T07:20:30Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 10, 2008 at 04:00:44PM -0800, Junio C Hamano wrote:\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > * add --strict option to unpack-objects (patch 1,8,9)\n> > * add --strict option to index-pack (patch 8,10)\n> >\n> > Same as for unpack-objects, but without writting objects.\n> >\n> > * add config option for receive pack to enable checking (patch 11)\n> \n> If this patch is any good, I strongly suspect it should not be\n> just the default but should always be on. IOW no config is\n> necessary. That would make the series a bit simpler, I guess.\n\nNot very much. In patch 11, we would not need to parse a config\nvariable and could pass --strict option unconditional to\nindex-pack/unpack-objects.\n\nFor the moment, I would like to keep it as an option (enabled by\ndefault). If somebody has a repository with a totally broken history,\nhe would not be able to push any more, if there is no way to disable\nit.\n\n> > From 76e86fe55345e633c910d6b8fe166e27c23c5aaf Mon Sep 17 00:00:00 2001\n> > From: Martin Koegler \n> > Date: Fri, 8 Feb 2008 08:51:38 +0100\n> > Subject: [PATCH 01/12] unpack-object: cache for non written objects\n> >\n> > Preventing objects with broken links entering the repository\n> > means, that write of some objects must be delayed.\n> >\n> > This patch adds a cache to keep the object data in memory. The delta\n> > resolving code must also search in the cache.\n> \n> I have to wonder what the memory pressure in real-life usage\n> will be like.\n> \n> When an object is proven to be good, we should be able to free\n> its buffer after writing it out, but would that be a good enough\n> optimization we can make later on this code to keep its memory\n> consumption manageable?\n\nThis code is only used by unpack-objects, which is used for small\npacks. It only caches metadata (tree,commit,tag), no blobs. So the\nmemory usage should not be a problem.\n\n> > diff --git a/fsck.c b/fsck.c\n> > new file mode 100644\n> > index 0000000..089f775\n> > --- /dev/null\n> > +++ b/fsck.c\n> > @@ -0,0 +1,84 @@\n> > +#include \"cache.h\"\n> > +#include \"object.h\"\n> > +#include \"blob.h\"\n> > +#include \"tree.h\"\n> > +#include \"tree-walk.h\"\n> > +#include \"commit.h\"\n> > +#include \"tag.h\"\n> > +#include \"fsck.h\"\n> > +\n> > +static int fsck_walk_tree(struct tree* tree, fsck_walk_func walk, void* data)\n> > +{\n> > +\tstruct tree_desc desc;\n> > +\tstruct name_entry entry;\n> > +\n> > +\tif(parse_tree(tree))\n> > +\t\treturn -1;\n> \n> It's a bit hard to see how these new set of functions relate to\n> the original code in this patch series, because you add the new\n> things that are initially not used anywhere independently, start\n> referring to them in a separate patch and then remove the old\n> related functions that are now unused. This style makes\n> reviewing easier and harder at the same time...\n\nWill try to restructure the patches.\n\n> > From 80b22c3f2c3e13c207790a49646020c55b34bba7 Mon Sep 17 00:00:00 2001\n> > From: Martin Koegler \n> > Date: Fri, 8 Feb 2008 09:01:50 +0100\n> > Subject: [PATCH 03/12] fsck: move mark-reachable to fsck_walk\n> >\n> > Signed-off-by: Martin Koegler \n> > ---\n> > builtin-fsck.c | 34 ++++++++++++++++++++++++----------\n> > 1 files changed, 24 insertions(+), 10 deletions(-)\n> > ...\n> > +static int mark_object(struct object* obj, int type, void* data)\n> > +{\n> > +\tif (!obj)\n> > +\t\treturn 0;\n> > +\tif (obj->flags & REACHABLE)\n> > +\t\treturn 0;\n> > +\tobj->flags |= REACHABLE;\n> > +\tif (!obj->parsed)\n> > +\t\treturn 0;\n> > +\treturn fsck_walk(obj, mark_object, data);\n> > +}\n> \n> Hmm. The return value 0 means Ok and negative is error? The\n> reason we can say success if obj is NULL or it is not parsed yet\n> is because...?\n\nIn mark_objects, I don't care for the results. Its should only\nmark all reachable objects (without crashing).\n\n> > @@ -326,8 +344,6 @@ static int fsck_tree(struct tree *item)\n> > \t\to_name = name;\n> > \t\to_sha1 = sha1;\n> > \t}\n> > -\tfree(item->buffer);\n> > -\titem->buffer = NULL;\n> \n> Hmm. The reason you still need the buffer after you checked the\n> contents of the tree in the loop is because you haven't actually\n> checked the referents are Ok. But I do not see a corresponding\n> free that releases this memory after you are actually done with\n> the verification with fsck_walk() yet, so we leak this in the\n> meantime?\n\nThe tree is traversed multiple times:\n* to mark reachable objects\n* to mark it's childs used\n* to check for broken links\n\nThe last use of the tree content is, after all objects are already in\nmemory.\n\n> > @@ -375,8 +391,6 @@ static int fsck_commit(struct commit *commit)\n> > \t}\n> > \tif (memcmp(buffer, \"author \", 7))\n> > \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\n> > -\tfree(commit->buffer);\n> > -\tcommit->buffer = NULL;\n> > \tif (!commit->tree)\n> > \t\treturn objerror(&commit->object, \"could not load commit's tree %s\", tree_sha1);\n> > \tif (!commit->parents && show_root)\n> \n> Likewise.\n\nWill change.\n\n> > From ce43251ef71962ff64fe138f1295c405ef6aaf65 Mon Sep 17 00:00:00 2001\n> > From: Martin Koegler \n> > Date: Fri, 8 Feb 2008 09:04:08 +0100\n> > Subject: [PATCH 04/12] fsck: move reachable object check to fsck_walk\n> >\n> > It handles NULL pointers in object references without crashing.\n> >\n> > Signed-off-by: Martin Koegler \n> > ---\n> > builtin-fsck.c | 49 +++++++++++++++++++++++++++++--------------------\n> > 1 files changed, 29 insertions(+), 20 deletions(-)\n> >\n> > diff --git a/builtin-fsck.c b/builtin-fsck.c\n> > index 49e96ff..2c1e10f 100644\n> > --- a/builtin-fsck.c\n> > +++ b/builtin-fsck.c\n> > @@ -81,13 +81,39 @@ static int objwarning(struct object *obj, const char *err, ...)\n> > \treturn -1;\n> > }\n> > \n> > +static int check_reachable_object_childs(struct object *obj, int type, void *data)\n> > +{\n> > +\tstruct object *parent = data;\n> > +\tif (!obj) {\n> > +\t\tprintf(\"broken link from %7s %s\\n\",\n> > +\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n> > +\t\tprintf(\"broken link from %7s %s\\n\",\n> > +\t\t\t (type==OBJ_ANY?\"unknown\":typename(type)), \"unknown\");\n> \n> Hmm? I am not sure what this part is reporting...\n\nIf eg. the sha1 of a commit is stored as tree in a commit, you will\nfind a null pointer in struct commit->tree. Such things will hit this\ncheck.\n\n> > From ee11f771be1ef1c29725cb56ab3eb8dfe61ca25a Mon Sep 17 00:00:00 2001\n> > From: Martin Koegler \n> > Date: Fri, 8 Feb 2008 09:07:33 +0100\n> > Subject: [PATCH 06/12] create a common object checker code out of fsck\n> >\n> > The function provides a callback for reporting errors.\n> \n> The same \"add unused new stuff independently, later use it and\n> then finally remove now unused old stuff\" pattern is here. I am\n> neutral to that patch style but it is a bit harder to see what\n> is going on.\n> \n> Most of the changes seem to be straight and sane copy-and-paste though.\n\nI'll merge this patch with the next patch.\n\n> > From a8db4e754e717bac0b2462333d4145eac3452099 Mon Sep 17 00:00:00 2001\n> > From: Martin Koegler \n> > Date: Fri, 8 Feb 2008 09:14:14 +0100\n> > Subject: [PATCH 09/12] unpack-objects: prevent writing of inconsistent objects\n> >\n> > This patch introduces a strict mode, which ensures that:\n> > - no malformed object will be written\n> > - no object with broken links will be written\n> >\n> > The patch ensures this by delaying the write of all non blob object.\n> > These object are written, after all objects they link to are written.\n> >\n> > An error can only result in unreferenced objects.\n> \n> > diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\n> > index f18c7e8..3e906e4 100644\n> > --- a/builtin-unpack-objects.c\n> > +++ b/builtin-unpack-objects.c\n> > @@ -173,7 +250,6 @@ static void resolve_delta(unsigned nr, enum object_type type,\n> > \t\tdie(\"failed to apply delta\");\n> > \tfree(delta);\n> > \twrite_object(nr, type, result, result_size);\n> > -\tfree(result);\n> > }\n> \n> And this is freed later elsewhere?\n\nThe free is the task of write_object. If (!strict), then it is freeed\nimmediatly. Similar for blobs. Other objects are put in the cache (see\npatch 1).\n\n> > @@ -203,7 +279,8 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n> > \n> > \tif (!dry_run && buf)\n> > \t\twrite_object(nr, type, buf, size);\n> > -\tfree(buf);\n> > +\telse if (buf)\n> > +\t\tfree(buf);\n> > }\n> \n> You can always free NULL without checking.\n\nWill fix.\n\n> > @@ -356,6 +434,7 @@ static void unpack_all(void)\n> > \tif (!quiet)\n> > \t\tprogress = start_progress(\"Unpacking objects\", nr_objects);\n> > \tobj_list = xmalloc(nr_objects * sizeof(*obj_list));\n> > +\tmemset(obj_list, 0, nr_objects * sizeof(*obj_list));\n> \n> Hmm, is this a fix to the 'master' independent from all the rest\n> of your patches, or a new requirement?\n\nThis is, because the new field in obj_list must be initialized with\nzero.\n\nmfg martin Kögler\n"},{"id":"68528","messageId":"alpine.LFD.1.00.0802120937330.2732@xanadu.home","threadId":"12011","inReplyTo":"20080211215806.GA24971@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-12T16:02:06Z","receivedAt":"2008-02-12T16:02:06Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 11 Feb 2008, Martin Koegler wrote:\n\n> On Mon, Feb 11, 2008 at 03:41:06PM -0500, Nicolas Pitre wrote:\n> > On Mon, 11 Feb 2008, Martin Koegler wrote:\n> > > Please remember, that --strict is used for pushing data.\n> > \n> > And what exactly does it provide in terms of safetiness that index-pack \n> > doesn't already do? I'm not sure I fully understood the goal here.\n> \n> The patch is about verifing the content of objects. My intented use is\n> for running a (restricted) server, which allows pushing via git-daemon\n> (or git-shell) from (untrusted) users. It can be also used to catch\n> corrupt objects, before they leave your repository, but this is for me\n> only a side effect.\n> \n> receive-pack accepts any crap (anything, which you can pipe into\n> git-hash-object with -t parameter of your choice), if the pack file\n> format is correct.\n> \n> But lots of code in git assums that the object content is welformd.\n> \n> Having such objects somewhere reachable in your repository will\n> disturb cloning (In the best case you get a error messages, in the\n> worst a child process of upload-pack segfaults), gitweb, ... . To fix\n> it, you will need a person with native access to the repository in the\n> worst case. \n\nOK that makes sense.\n\nI think this is a good idea to always have some sanity checks on any \nincoming objects so to make sure they're well formed and valid before \ngiving them a SHA1 value, and bail out as soon as any error is found. \nFrom my understanding that's what your patch is doing, right? (sorry I \ncan't find them in my mailbox anymore). This can be done as objects are \ncoming in just fine and requires no extra memory, and I would say this \nshould be done unconditionally all the time. After all, the Git \ncoherency model is based on the SHA1 checksuming, and therefore it is a \ngood idea to never validate any malformed objects with a SHA1. So I'm \nall in favor of such validation always performed in index-pack and \nunpack-objects.\n\nAs to making sure those objects are well connected... well this is a \ntechnically different issue entirely, and I wonder if a special mode to \nfsck might not be a better solution. For example, fsck could be made to \nvalidate object connectivity, starting from the new ref(s), and stopping \nobject walking as soon as a reference to an object not included in the \nnewly received pack is encountered. This could be run from some hook to \ndecide whether or not to update the new refs, and to delete the pack \notherwise.\n\n\nNicolas\n"},{"id":"68540","messageId":"20080212190411.GA23837@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802120937330.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-12T19:04:11Z","receivedAt":"2008-02-12T19:04:11Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Tue, Feb 12, 2008 at 11:02:06AM -0500, Nicolas Pitre wrote:\n> I think this is a good idea to always have some sanity checks on any \n> incoming objects so to make sure they're well formed and valid before \n> giving them a SHA1 value, and bail out as soon as any error is found. \n> From my understanding that's what your patch is doing, right? (sorry I \n> can't find them in my mailbox anymore). \n\nYes. (=>http://marc.info/?l=git&m=120266631524947&w=2)\n\n> This can be done as objects are \n> coming in just fine and requires no extra memory, and I would say this \n> should be done unconditionally all the time. After all, the Git \n> coherency model is based on the SHA1 checksuming, and therefore it is a \n> good idea to never validate any malformed objects with a SHA1. So I'm \n> all in favor of such validation always performed in index-pack and \n> unpack-objects.\n\nWe will need some additional memory for struct blob/tree/tag/commit\neven for this check.\n\nI'll start reworking my patches.\n\n> As to making sure those objects are well connected... well this is a \n> technically different issue entirely, and I wonder if a special mode to \n> fsck might not be a better solution. For example, fsck could be made to \n> validate object connectivity, starting from the new ref(s), and stopping \n> object walking as soon as a reference to an object not included in the \n> newly received pack is encountered. This could be run from some hook to \n> decide whether or not to update the new refs, and to delete the pack \n> otherwise.\n\nDo you really think, that this will need less memory? fsck loads first\nall objects and then verifies their connections.\n\nmfg Martin Kögler\n"},{"id":"68544","messageId":"alpine.LFD.1.00.0802121507310.2732@xanadu.home","threadId":"12011","inReplyTo":"20080212190411.GA23837@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-12T20:22:17Z","receivedAt":"2008-02-12T20:22:17Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 12 Feb 2008, Martin Koegler wrote:\n\n> On Tue, Feb 12, 2008 at 11:02:06AM -0500, Nicolas Pitre wrote:\n> > I think this is a good idea to always have some sanity checks on any \n> > incoming objects so to make sure they're well formed and valid before \n> > giving them a SHA1 value, and bail out as soon as any error is found. \n> > From my understanding that's what your patch is doing, right? (sorry I \n> > can't find them in my mailbox anymore). \n> \n> Yes. (=>http://marc.info/?l=git&m=120266631524947&w=2)\n> \n> > This can be done as objects are \n> > coming in just fine and requires no extra memory, and I would say this \n> > should be done unconditionally all the time. After all, the Git \n> > coherency model is based on the SHA1 checksuming, and therefore it is a \n> > good idea to never validate any malformed objects with a SHA1. So I'm \n> > all in favor of such validation always performed in index-pack and \n> > unpack-objects.\n> \n> We will need some additional memory for struct blob/tree/tag/commit\n> even for this check.\n\nWhy so?\n\nEach received object is stored in memory when received, so why can't you \nsimply validate it in place? No need to keep trace of them afterward.\n\n> > As to making sure those objects are well connected... well this is a \n> > technically different issue entirely, and I wonder if a special mode to \n> > fsck might not be a better solution. For example, fsck could be made to \n> > validate object connectivity, starting from the new ref(s), and stopping \n> > object walking as soon as a reference to an object not included in the \n> > newly received pack is encountered. This could be run from some hook to \n> > decide whether or not to update the new refs, and to delete the pack \n> > otherwise.\n> \n> Do you really think, that this will need less memory? fsck loads first\n> all objects and then verifies their connections.\n\nNot all objects otherwise I wouldn't even be able to run it.\n\nMy point is that you can have fsck load only objects contained in the \nreceived pack (you can use the pack index to load them) and assume \nconnectivity is good whenever an object in the pack reference an \nexisting object outside of the pack. At least this doesn't need to \nhappen in parallel with pack indexing.\n\n\nNicolas\n"},{"id":"68553","messageId":"20080212213858.GA29151@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802121507310.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-12T21:38:58Z","receivedAt":"2008-02-12T21:38:58Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Tue, Feb 12, 2008 at 03:22:17PM -0500, Nicolas Pitre wrote:\n> On Tue, 12 Feb 2008, Martin Koegler wrote:\n> > We will need some additional memory for struct blob/tree/tag/commit\n> > even for this check.\n> \n> Why so?\n> \n> Each received object is stored in memory when received, so why can't you \n> simply validate it in place? No need to keep trace of them afterward.\n\nFreeing the data is not my problem.\n\nMany validations are in parse_XXX_buffer, which are also used by\nfsck. This returns a struct commit/tree/tag/blob.\n\nI have not found any code in git to free them. \n\nSame for pack-objects, e.g. add_objects_in_unpacked_packs allocates\nmany struct object via lookup_unknown_object. As far as I understand\nthe code, they are never freed, even if they are not needed later.\n\n> > > As to making sure those objects are well connected... well this is a \n> > > technically different issue entirely, and I wonder if a special mode to \n> > > fsck might not be a better solution. For example, fsck could be made to \n> > > validate object connectivity, starting from the new ref(s), and stopping \n> > > object walking as soon as a reference to an object not included in the \n> > > newly received pack is encountered. This could be run from some hook to \n> > > decide whether or not to update the new refs, and to delete the pack \n> > > otherwise.\n> > \n> > Do you really think, that this will need less memory? fsck loads first\n> > all objects and then verifies their connections.\n> \n> Not all objects otherwise I wouldn't even be able to run it.\n\nIt only loads all objects, it checks (eg. no objects from the pack file\nby default). After loading a object, it frees the content of object,\nbut keeps the parsed information in memory for the connectivity check.\n\n> My point is that you can have fsck load only objects contained in the \n> received pack (you can use the pack index to load them) and assume \n> connectivity is good whenever an object in the pack reference an \n> existing object outside of the pack. At least this doesn't need to \n> happen in parallel with pack indexing.\n\nSo you propose the following:\n1) run index-pack (write pack file + index in repository)\n2) fsck pack file\n3) update refs\n\nThis looks to be very prone to race conditions, if multiple\npushes run concurrently.\n\nTo be on the safe side, the checks must be finished, before a pack/object\nbecomes part of the repository. We must check the reachability before \nindex-pack writes the index.\n\nIn my opinion, separating the code for the reachability check from\nindex-pack would complicate things. It would not safe memory, as the\nmemory would be used by two processes instead of one.\n\nYou don't want to increase the resource usage of index-pack. If\n--strict is not passed, I implemented this. \n\nFor --strict, it is not avoidable, that index-pack will use more\nmemory and cpu time.\n\nmfg Martin Kögler\n"},{"id":"68556","messageId":"alpine.LFD.1.00.0802121645121.2732@xanadu.home","threadId":"12011","inReplyTo":"20080212213858.GA29151@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-12T21:51:12Z","receivedAt":"2008-02-12T21:51:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 12 Feb 2008, Martin Koegler wrote:\n\n> On Tue, Feb 12, 2008 at 03:22:17PM -0500, Nicolas Pitre wrote:\n> > On Tue, 12 Feb 2008, Martin Koegler wrote:\n> > > We will need some additional memory for struct blob/tree/tag/commit\n> > > even for this check.\n> > \n> > Why so?\n> > \n> > Each received object is stored in memory when received, so why can't you \n> > simply validate it in place? No need to keep trace of them afterward.\n> \n> Freeing the data is not my problem.\n\nWell, I would say it is now. ;-)\n\n> Many validations are in parse_XXX_buffer, which are also used by\n> fsck. This returns a struct commit/tree/tag/blob.\n> \n> I have not found any code in git to free them. \n\nMaybe we should add it then. Especially in the usual case where we want \nincoming objects to be validated.\n\n> Same for pack-objects, e.g. add_objects_in_unpacked_packs allocates\n> many struct object via lookup_unknown_object. As far as I understand\n> the code, they are never freed, even if they are not needed later.\n\nHmmm, that's bad.\n\n\nNicolas\n"},{"id":"68601","messageId":"20080213062015.GF24004@spearce.org","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802121645121.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-13T06:20:15Z","receivedAt":"2008-02-13T06:20:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Tue, 12 Feb 2008, Martin Koegler wrote:\n> \n> > Many validations are in parse_XXX_buffer, which are also used by\n> > fsck. This returns a struct commit/tree/tag/blob.\n> > \n> > I have not found any code in git to free them. \n\nYea, there isn't a way to free the struct commit/tree/tag/blob\ncurrently. Once allocated its inside the hash, cannot be removed,\nand cannot be recycled.\n\nA while back I had worked up a patch that allowed us to reset the\nentire hash and recycle the all of the objects, but that isn't\nreally what we need here.\n \n> Maybe we should add it then. Especially in the usual case where we want \n> incoming objects to be validated.\n\nReleasing a single object back shouldn't be too difficult, so long as\nyou release everything after each parse. Remember parsing a commit\nloads the parent list with struct commit*s. Those are in the object\nhash and a later parse_commit_buffer() may attempt to reuse those\nif the same commit was a parent for more than one descendent. :-\\\n\nBasically I think you want a smaller case of what I had done earlier\nin my attempt to fix an error in git-fetch; release everything in\nthe object pool after each object has been parsed and validated.\nWe should only have a couple of objects allocated in the blocks\n(one blob, one tree, one tag, handful of commits) and just reset\nthe blocks and hash between each.\n \n> > Same for pack-objects, e.g. add_objects_in_unpacked_packs allocates\n> > many struct object via lookup_unknown_object. As far as I understand\n> > the code, they are never freed, even if they are not needed later.\n> \n> Hmmm, that's bad.\n\nYou can't free them. We're using their o->flags field to store\nOBJECT_ADDED state so we don't pack the same object twice in our\noutput, even if it appears in multiple input packfiles.\n\nThis isn't a leak. Its behaving as designed. It is however\noverallocating the object, because we are allocating an unknown\nobject rather than a specific object type. We don't know what the\nheck that SHA-1 is just from looking at the index. If it is an\nOBJ_{REF,OFS}_DELTA its too expensive to acquire the exact type\nduring this method.\n\nWhich makes me wonder, would it make sense to store the object type\ncode inside of the .idx?\n\nThere's a few places in Git (4 lookup_unknown_objects, 19 possible\nsha1_object_infos) where we may be able to benefit from having fast\naccess to the base type for an object, without needing to walk its\ndelta chain to acquire that information. It certainly would help\nin this spot of pack-objects that Martin has pointed out.\n\nBut I really don't think it is worth the disk storage space\nnecessary. We only need 3 bits, but there isn't a place to steal\nthem from in the index v2 file format. So we're adding a full byte\nper object. Further we only make this lookup_unknown_object call\nin 3 locations:\n\n http-push.c/walker.c:\n\n We're fetching objects, but we don't know what we are getting.\n I think these are only triggered during the loose object\n upload/download, which should be only a fraction of the total\n objects transferred, and are only for cases where the object\n isn't local so we can't check its type before allocation.\n\n No big deal.\n\n\n tree.c:\n\n This is inside track_tree_refs and its a file mode we do\n not recognize. We spit out a warning, but try to lookup\n the unknown object anyway, even though we're looking at what\n should be a bogus tree. This occurs only in parse_tree_buffer,\n only if track_objects_refs is on, and only if we see a tree\n that is actually not understood by this version of Git. Hmm,\n shouldn't happen unless the tree itself is corrupt.\n\n This code looked fishy enough to me to dig up the blame history\n for it:\n\n commit e2ac7cb5fbcf1407003aa07cdcd14141527ea2e3\n Author: Sam Vilain \n Date: Wed Jun 6 06:25:17 2007\n\n Don't assume tree entries that are not dirs are blobs\n\n When scanning the trees in track_tree_refs() there is a \"lazy\" test\n that assumes that entries are either directories or files. Don't do\n that.\n\n Sounds like it was around the time of S_ISGITLINK being\n introduced. Looking at this code again I have to wonder, why\n the hell are we looking up and tracking an object inside of a\n tree when we don't understand the mode?\n\n Lets say a new form of S_ISGITLINK gets introduced in the future.\n By this I mean the mode says \"hey, this SHA-1 isn't really in my\n object pool\". We're going to cram a dummy object into the refs\n table for this tree. Why? We don't do this for S_ISGITLINK.\n\n Lets say its a new object type (not tree/tag/commit/blob) but\n it is in our storage pool. If this Git doesn't know the mode,\n it sure as heck won't know what the hell the loose object header\n (or pack header!) says about that object.\n\n One of the key places where you might expect tracking an unknown\n (but referenced by a tree) SHA-1 type would be in reachability,\n rev-list --objects, packfile generation. But the process_tree()\n function in reachable.c doesn't have Sam's change above, so it\n will assume anything new looking is a blob.\n \n Oh, and what a rabbit hole I just fell down. The only caller\n that seems to set \"track_object_refs = 1\" (and thus get into this\n odd lookup_unknown_object() call) is fsck. Everyone else sets\n it to 0, including its default declaration.\n\n So we've got this nice baggage, and differing implementation,\n so fsck can be happy how? We've also got a whole lot of apps\n setting \"track_object_refs = 0\", which is what it defaults to,\n unless you managed to run fsck first. Hmmph.\n\n Is it just me or is track_object_refs perhaps outlived its\n usefulness?\n\n pack-objects:\n\n Here we are packing everything in an existing packfile, as we\n are doing something like a \"git repack -A\" and need to carry\n it all with us.\n\n We probably could be smarter than allocating an unknown object,\n but I'm not sure its worth it. If you really are paying a\n high price here it means you are carrying a *huge* amount of\n unreachable garbage with you from packfile to packfile.\n\n Try running \"git gc --prune\" every few months.\n \n-- \nShawn.\n"},{"id":"68604","messageId":"20080213073959.GA27158@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"20080213062015.GF24004@spearce.org","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-13T07:39:59Z","receivedAt":"2008-02-13T07:39:59Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Wed, Feb 13, 2008 at 01:20:15AM -0500, Shawn O. Pearce wrote:\n> tree.c:\n> \n> This is inside track_tree_refs and its a file mode we do\n> not recognize. We spit out a warning, but try to lookup\n> the unknown object anyway, even though we're looking at what\n> should be a bogus tree. This occurs only in parse_tree_buffer,\n> only if track_objects_refs is on, and only if we see a tree\n> that is actually not understood by this version of Git. Hmm,\n> shouldn't happen unless the tree itself is corrupt.\n> \n> This code looked fishy enough to me to dig up the blame history\n> for it:\n> \n> commit e2ac7cb5fbcf1407003aa07cdcd14141527ea2e3\n> Author: Sam Vilain \n> Date: Wed Jun 6 06:25:17 2007\n> \n> Don't assume tree entries that are not dirs are blobs\n> \n> When scanning the trees in track_tree_refs() there is a \"lazy\" test\n> that assumes that entries are either directories or files. Don't do\n> that.\n> \n> Sounds like it was around the time of S_ISGITLINK being\n> introduced. Looking at this code again I have to wonder, why\n> the hell are we looking up and tracking an object inside of a\n> tree when we don't understand the mode?\n> \n> Lets say a new form of S_ISGITLINK gets introduced in the future.\n> By this I mean the mode says \"hey, this SHA-1 isn't really in my\n> object pool\". We're going to cram a dummy object into the refs\n> table for this tree. Why? We don't do this for S_ISGITLINK.\n> \n> Lets say its a new object type (not tree/tag/commit/blob) but\n> it is in our storage pool. If this Git doesn't know the mode,\n> it sure as heck won't know what the hell the loose object header\n> (or pack header!) says about that object.\n> \n> One of the key places where you might expect tracking an unknown\n> (but referenced by a tree) SHA-1 type would be in reachability,\n> rev-list --objects, packfile generation. But the process_tree()\n> function in reachable.c doesn't have Sam's change above, so it\n> will assume anything new looking is a blob.\n> \n> Oh, and what a rabbit hole I just fell down. The only caller\n> that seems to set \"track_object_refs = 1\" (and thus get into this\n> odd lookup_unknown_object() call) is fsck. Everyone else sets\n> it to 0, including its default declaration.\n> \n> So we've got this nice baggage, and differing implementation,\n> so fsck can be happy how? We've also got a whole lot of apps\n> setting \"track_object_refs = 0\", which is what it defaults to,\n> unless you managed to run fsck first. Hmmph.\n> \n> Is it just me or is track_object_refs perhaps outlived its\n> usefulness?\n\nSee the patch at the start of this topic. I'm working on replacing\ntrack-object-refs.\n\nIt is missing some frees and I copied the lookup_unknown_object call\n(This should be replaced with an error message in my patch, as the the\nverification of the tree content would reject such an invalid mode).\n\nmfg Martin Kögler\n"},{"id":"68605","messageId":"20080213074209.GG24004@spearce.org","threadId":"12011","inReplyTo":"alpine.LFD.1.00.0802120937330.2732@xanadu.home","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-13T07:42:09Z","receivedAt":"2008-02-13T07:42:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Mon, 11 Feb 2008, Martin Koegler wrote:\n> > \n> > But lots of code in git assums that the object content is welformd.\n> > \n> > Having such objects somewhere reachable in your repository will\n> > disturb cloning (In the best case you get a error messages, in the\n> > worst a child process of upload-pack segfaults), gitweb, ... . To fix\n> > it, you will need a person with native access to the repository in the\n> > worst case. \n\nWe have *waaaay* overthought this. The solution is a lot simpler\nthan Martin's patches make it out to be, and we can do it without\nchanging our current memory footprint.\n\n> I think this is a good idea to always have some sanity checks on any \n> incoming objects so to make sure they're well formed and valid before \n> giving them a SHA1 value, and bail out as soon as any error is found. \n\nWhen we get the raw data for an object so we can compute its SHA-1\nand/or write its loose object to disk we should first verify its\ncontent is sane, then do the SHA-1/store loose.\n\nBlobs - always assume sane. Remember that these make up the largest\npercentage in terms of raw bytes of any project's packfiles and we\ndon't need to do any additional processing to them. Yay us.\n\nTags - stack allocate a temporary struct tag and pass it's address\ninto parse_tag_buffer(). The only thing it tries to allocate is\nthe item->tagged. Throw an extra argument into parse_tag_buffer()\nto bypass this lookup block. Now there's no extra memory used for\ntags, just a bit more CPU. Tags aren't frequent.\n\nCommits - do like tags, but use parse_commit_buffer(). Avoid\nlooking up the tree object and the parents, and allocating the\nparent references via a new argument. Also check !item->date like\nfsck does. Other than that I think the validation code in fsck is\nredundant with what parse_commit_buffer() is doing already.\n\nTrees - just run init_tree_desc() and tree_entry() loop like in\ntrack_tree_refs() (and a ton of other places) to iterate over the\nbuffer. All we care about is sane entry names (no '/'), sane modes,\nand correct sorting.\n\nIn short, we ignore reachability, but get fsck, and we can completely\navoid additional malloc activity as well as any sort of heap increase\nin index-pack and unpack-objects. Its not hard.\n\nIts a small amount of refactoring for the parse functions we\nalready have, and maybe expose a function or two from builtin-fsck\n(in particular a bulk of fsck_tree should be reused).\n\nYea, its a tad more CPU time, but it shouldn't be that costly here.\nThe huge cost in fsck is redoing the SHA-1 checksums, and inflating\nthe deltas. We've already got the delta inflated, and we're about\nto compute the SHA-1 checksum for the first time. So those major\ncosts of fsck drop to 0.\n\n> As to making sure those objects are well connected... well this is a \n> technically different issue entirely, and I wonder if a special mode to \n> fsck might not be a better solution.\n\nNah, just do what quickfetch does in builtin-fetch.c, but run it\nin receive-pack, between unpack() and execute_commands():\n\n\trev-list --quiet --objects $new... --not --all\n\nIf it aborts, reachability testing failed and the push is rejected\nwithout updating any refs. Yes your repository now has objects\nthat are missing things, but none of those are considered to be\nreachable, so this isn't a big deal. They will get cleaned up on\nthe next `gc --prune`, whenever that is.\n\nIn this configuration (--quiet) rev-list tries to be pretty low\non its memory usage, it doesn't save buffers, etc. Further since\neverything that is already considered reachable is not interesting,\nwe are only doing a walk over the objects that we just received,\nnot our entire ODB. Its also after index-pack exited, so we just\nfreed up a good chunk of memory.\n\nRememeber we are talking about receive-pack here. The cost on\nthe to perform the rev-list is lower than the cost will be to pack\nthese objects for distribution back to just one client. Since this\nis a server of some sorts (otherwise why did you push here?), odds\nare its going to be doing a lot of packing requests for clients to\nreceive these newly uploaded objects by the native git protocol.\nThis new rev-list is nothing compared to that already existing load.\nAnd if your OS is any good the just created .idx and .pack is still\nin OS buffer cache, so there shouldn't be any additional disk IO.\n\nYes, we could make this optional in receive-pack, but really I don't\nsee a reason to. Just run it. The client shouldn't be giving us\nunreachable crap.\n\n-- \nShawn.\n"},{"id":"68608","messageId":"20080213081128.GA27730@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"20080213074209.GG24004@spearce.org","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-13T08:11:29Z","receivedAt":"2008-02-13T08:11:29Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Wed, Feb 13, 2008 at 02:42:09AM -0500, Shawn O. Pearce wrote:\n> Nicolas Pitre wrote:\n> > I think this is a good idea to always have some sanity checks on any \n> > incoming objects so to make sure they're well formed and valid before \n> > giving them a SHA1 value, and bail out as soon as any error is found. \n> \n> When we get the raw data for an object so we can compute its SHA-1\n> and/or write its loose object to disk we should first verify its\n> content is sane, then do the SHA-1/store loose.\n> \n[...]\n> > As to making sure those objects are well connected... well this is a \n> > technically different issue entirely, and I wonder if a special mode to \n> > fsck might not be a better solution.\n> \n> Nah, just do what quickfetch does in builtin-fetch.c, but run it\n> in receive-pack, between unpack() and execute_commands():\n> \n> \trev-list --quiet --objects $new... --not --all\n> \n> If it aborts, reachability testing failed and the push is rejected\n> without updating any refs. Yes your repository now has objects\n> that are missing things, but none of those are considered to be\n> reachable, so this isn't a big deal. They will get cleaned up on\n> the next `gc --prune`, whenever that is.\n\nThis would mean, that we must make git-rev-list and git-pack-objects\nnot segfault on incorrect links between objects.\n\n> In this configuration (--quiet) rev-list tries to be pretty low\n> on its memory usage, it doesn't save buffers, etc. Further since\n> everything that is already considered reachable is not interesting,\n> we are only doing a walk over the objects that we just received,\n> not our entire ODB. Its also after index-pack exited, so we just\n> freed up a good chunk of memory.\n> \n> Rememeber we are talking about receive-pack here. The cost on\n> the to perform the rev-list is lower than the cost will be to pack\n> these objects for distribution back to just one client. Since this\n> is a server of some sorts (otherwise why did you push here?), odds\n> are its going to be doing a lot of packing requests for clients to\n> receive these newly uploaded objects by the native git protocol.\n> This new rev-list is nothing compared to that already existing load.\n> And if your OS is any good the just created .idx and .pack is still\n> in OS buffer cache, so there shouldn't be any additional disk IO.\n> \n> Yes, we could make this optional in receive-pack, but really I don't\n> see a reason to. Just run it. The client shouldn't be giving us\n> unreachable crap.\n\nLooks sane to me.\n\nmfg Martin Kögler\n"},{"id":"68636","messageId":"alpine.LSU.1.00.0802131200410.30505@racer.site","threadId":"12011","inReplyTo":"20080213081128.GA27730@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-13T12:01:43Z","receivedAt":"2008-02-13T12:01:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 13 Feb 2008, Martin Koegler wrote:\n\n> This would mean, that we must make git-rev-list and git-pack-objects not \n> segfault on incorrect links between objects.\n\nWe should do that anyway. It may error out, but segfaulting is no option.\n\nSo if you have a test case, please make it public so we can fix the \nbreakage.\n\nCiao,\nDscho\n"},{"id":"68705","messageId":"20080214061651.GH24004@spearce.org","threadId":"12011","inReplyTo":"alpine.LSU.1.00.0802131200410.30505@racer.site","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-14T06:16:51Z","receivedAt":"2008-02-14T06:16:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> On Wed, 13 Feb 2008, Martin Koegler wrote:\n> \n> > This would mean, that we must make git-rev-list and git-pack-objects not \n> > segfault on incorrect links between objects.\n\nMy proposal suggested that malformed objects not be allowed\ninto the repository, so rev-list wouldn't see them in the\nfirst place.\n \n> We should do that anyway. It may error out, but segfaulting is no option.\n> \n> So if you have a test case, please make it public so we can fix the \n> breakage.\n\nBut Dscho has a really good point here. rev-list should be failing\nwith a proper exit code, not SIGSEGV. :)\n\n-- \nShawn.\n"},{"id":"68723","messageId":"20080214090013.GK24004@spearce.org","threadId":"12011","inReplyTo":"20080213073959.GA27158@auto.tuwien.ac.at","subject":"[RFC PATCH] Remove object-refs from fsck","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-14T09:00:13Z","receivedAt":"2008-02-14T09:00:13Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Koegler wrote:\n> On Wed, Feb 13, 2008 at 01:20:15AM -0500, Shawn O. Pearce wrote:\n> > Is it just me or is track_object_refs perhaps outlived its\n> > usefulness?\n> \n> See the patch at the start of this topic. I'm working on replacing\n> track-object-refs.\n> \n> It is missing some frees and I copied the lookup_unknown_object call\n> (This should be replaced with an error message in my patch, as the the\n> verification of the tree content would reject such an invalid mode).\n\nHere's my first attempt at removing track_object_refs. At least\nthe diffstat suggets its in the correct direction. Note! It is\nonly very lightly tested, which is why this is not a properly\nformatted patch.\n\nI'm trying to reuse the code in reachable.c, which is what prune\nis based on. We now parse trees twice. Once to read the content\nand verify it is sane and later to perform the reachability walk.\nWe use less memory however as we no longer have object_refs.\n\nMissing objects discovered during fsck get flagged on the referrer\nby the new WANTING flag. We go back later and reparse the referrer\nto find out what links he had that were broken. This 3rd parse of\na tree shouldn't be an issue in a perfectly valid repository.\n\nIn other news, fsck --full is a dog.\n\nI think we can make it run faster for packed data by fsck'ing\naccording to the delta base, as index-pack does. We also could get\nbetter running time if we avoided the per-object unpack/checksum\nthat occurs in verify_pack() and instead do it as part of the fsck\ndelta base ordered loop.\n\nToday `fsck --full` is actually SHA-1 checksumming every object in\na packfile twice: once in verify_pack() and again in parse_object().\nCute.\n\n\n--8<--\n Makefile | 2 +-\n builtin-fetch-pack.c | 1 -\n builtin-fsck.c | 199 ++++++++++++++++++++++++++++++------------------\n builtin-pack-objects.c | 1 -\n builtin-prune.c | 2 +-\n builtin-reflog.c | 2 +-\n builtin-rev-list.c | 1 -\n commit.c | 11 ---\n object-refs.c | 87 ---------------------\n object.h | 13 ---\n reachable.c | 43 ++++++++--\n reachable.h | 7 ++-\n tag.c | 6 --\n tree.c | 48 ------------\n upload-pack.c | 1 -\n walker.c | 1 -\n 16 files changed, 168 insertions(+), 257 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 92341c4..8f930ed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -312,7 +312,7 @@ LIB_OBJS = \\\n \tpatch-ids.o \\\n \tobject.o pack-check.o pack-write.o patch-delta.o path.o pkt-line.o \\\n \tsideband.o reachable.o reflog-walk.o \\\n-\tquote.o read-cache.o refs.o run-command.o dir.o object-refs.o \\\n+\tquote.o read-cache.o refs.o run-command.o dir.o \\\n \tserver-info.o setup.o sha1_file.o sha1_name.o strbuf.o \\\n \ttag.o tree.o usage.o config.o environment.o ctype.o copy.o \\\n \trevision.o pager.o tree-walk.o xdiff-interface.o \\\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex f401352..25f1915 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -386,7 +386,6 @@ static int everything_local(struct ref **refs, int nr_match, char **match)\n \tint retval;\n \tunsigned long cutoff = 0;\n \n-\ttrack_object_refs = 0;\n \tsave_commit_buffer = 0;\n \n \tfor (ref = *refs; ref; ref = ref->next) {\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex cc7524b..50c358b 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -9,9 +9,12 @@\n #include \"cache-tree.h\"\n #include \"tree-walk.h\"\n #include \"parse-options.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"reachable.h\"\n \n-#define REACHABLE 0x0001\n-#define SEEN 0x0002\n+#define FSCKED (1u<<17)\n+#define WANTING (1u<<18)\n \n static int show_root;\n static int show_tags;\n@@ -63,13 +66,81 @@ static int objwarning(struct object *obj, const char *err, ...)\n \treturn -1;\n }\n \n+static int object_exists(const unsigned char *sha1)\n+{\n+\tstruct object *obj = lookup_object(sha1);\n+\tif (obj && obj->flags & FSCKED)\n+\t\treturn 1;\n+\treturn has_sha1_file(sha1);\n+}\n+\n+static void broken_link(struct object *obj,\n+\t\tenum object_type ref_type,\n+\t\tconst unsigned char *ref_sha1)\n+{\n+\tprintf(\"broken link from %7s %s\\n\",\n+\t typename(obj->type), sha1_to_hex(obj->sha1));\n+\tprintf(\" to %7s %s\\n\",\n+\t typename(ref_type), sha1_to_hex(ref_sha1));\n+\terrors_found |= ERROR_REACHABLE;\n+}\n+\n+static void broken_tree(struct tree *item)\n+{\n+\tstruct tree_desc desc;\n+\n+\titem->object.parsed = 0;\n+\tif (parse_tree(item) < 0)\n+\t\treturn; /* error already displayed */\n+\n+\tinit_tree_desc(&desc, item->buffer, item->size);\n+\twhile (desc.size) {\n+\t\tunsigned mode;\n+\t\tconst char *name;\n+\t\tconst unsigned char *sha1;\n+\n+\t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n+\t\tupdate_tree_entry(&desc);\n+\t\tif (S_ISGITLINK(mode) || object_exists(sha1))\n+\t\t\tcontinue;\n+\t\tif (S_ISDIR(mode))\n+\t\t\tbroken_link(&item->object, OBJ_TREE, sha1);\n+\t\telse\n+\t\t\tbroken_link(&item->object, OBJ_BLOB, sha1);\n+\t}\n+\tfree(item->buffer);\n+\titem->buffer = NULL;\n+}\n+\n+static void broken_commit(struct commit *commit)\n+{\n+\tstruct tree *tree = commit->tree;\n+\tstruct commit_list *pp;\n+\n+\tif (tree && !object_exists(tree->object.sha1))\n+\t\tbroken_link(&commit->object, OBJ_TREE, tree->object.sha1);\n+\n+\tpp = commit->parents;\n+\twhile (pp) {\n+\t\tstruct commit *p = pp->item;\n+\t\tif (p && !object_exists(p->object.sha1))\n+\t\t\tbroken_link(&commit->object, OBJ_COMMIT, p->object.sha1);\n+\t\tpp = pp->next;\n+\t}\n+}\n+\n+static void broken_tag(struct tag *tag)\n+{\n+\tstruct object *ref = tag->tagged;\n+\tif (ref && !object_exists(ref->sha1))\n+\t\tbroken_link(&tag->object, ref->type, ref->sha1);\n+}\n+\n /*\n * Check a single reachable object\n */\n static void check_reachable_object(struct object *obj)\n {\n-\tconst struct object_refs *refs;\n-\n \t/*\n \t * We obviously want the object to be parsed,\n \t * except if it was in a pack-file and we didn't\n@@ -84,21 +155,28 @@ static void check_reachable_object(struct object *obj)\n \t}\n \n \t/*\n-\t * Check that everything that we try to reference is also good.\n+\t * If the fsck routines found this object WANTING\n+\t * then we need to re-evaluate what it wanted and\n+\t * report on those broken links.\n \t */\n-\trefs = lookup_object_refs(obj);\n-\tif (refs) {\n-\t\tunsigned j;\n-\t\tfor (j = 0; j < refs->count; j++) {\n-\t\t\tstruct object *ref = refs->ref[j];\n-\t\t\tif (ref->parsed ||\n-\t\t\t (has_sha1_file(ref->sha1)))\n-\t\t\t\tcontinue;\n-\t\t\tprintf(\"broken link from %7s %s\\n\",\n-\t\t\t typename(obj->type), sha1_to_hex(obj->sha1));\n-\t\t\tprintf(\" to %7s %s\\n\",\n-\t\t\t typename(ref->type), sha1_to_hex(ref->sha1));\n-\t\t\terrors_found |= ERROR_REACHABLE;\n+\tif (obj->flags & WANTING) {\n+\t\tswitch (obj->type) {\n+\t\tcase OBJ_TREE:\n+\t\t\tbroken_tree((struct tree *) obj);\n+\t\t\tbreak;\n+\n+\t\tcase OBJ_COMMIT:\n+\t\t\tbroken_commit((struct commit *) obj);\n+\t\t\tbreak;\n+\n+\t\tcase OBJ_TAG:\n+\t\t\tbroken_tag((struct tag *) obj);\n+\t\t\tbreak;\n+\n+\t\tdefault:\n+\t\t\tobjerror(obj,\n+\t\t\t\t\"type '%d' has broken link (internal fsck error)\",\n+\t\t\t\tobj->type);\n \t\t}\n \t}\n }\n@@ -181,7 +259,7 @@ static void check_object(struct object *obj)\n \tif (verbose)\n \t\tfprintf(stderr, \"Checking %s\\n\", sha1_to_hex(obj->sha1));\n \n-\tif (obj->flags & REACHABLE)\n+\tif (obj->flags & SEEN)\n \t\tcheck_reachable_object(obj);\n \telse\n \t\tcheck_unreachable_object(obj);\n@@ -309,6 +387,15 @@ static int fsck_tree(struct tree *item)\n \t\t\thas_bad_modes = 1;\n \t\t}\n \n+\t\t/*\n+\t\t * If the SHA-1 should exist in this repository but\n+\t\t * we are missing it flag this tree as WANTING. We\n+\t\t * will revisit this error later once we determine\n+\t\t * this tree is SEEN.\n+\t\t */\n+\t\tif (!S_ISGITLINK(mode) && !object_exists(sha1))\n+\t\t\titem->object.flags |= WANTING;\n+\n \t\tif (o_name) {\n \t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n \t\t\tcase TREE_UNORDERED:\n@@ -367,10 +454,15 @@ static int fsck_commit(struct commit *commit)\n \t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n \t\treturn objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n+\tif (!object_exists(tree_sha1))\n+\t\tcommit->object.flags |= WANTING;\n+\n \tbuffer += 46;\n \twhile (!memcmp(buffer, \"parent \", 7)) {\n \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n \t\t\treturn objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n+\t\tif (!object_exists(sha1))\n+\t\t\tcommit->object.flags |= WANTING;\n \t\tbuffer += 48;\n \t}\n \tif (memcmp(buffer, \"author \", 7))\n@@ -395,6 +487,9 @@ static int fsck_tag(struct tag *tag)\n \tif (!tagged) {\n \t\treturn objerror(&tag->object, \"could not load tagged object\");\n \t}\n+\tif (!object_exists(tagged->sha1))\n+\t\ttag->object.flags |= WANTING;\n+\n \tif (!show_tags)\n \t\treturn 0;\n \n@@ -411,9 +506,9 @@ static int fsck_sha1(const unsigned char *sha1)\n \t\treturn error(\"%s: object corrupt or missing\",\n \t\t\t sha1_to_hex(sha1));\n \t}\n-\tif (obj->flags & SEEN)\n+\tif (obj->flags & FSCKED)\n \t\treturn 0;\n-\tobj->flags |= SEEN;\n+\tobj->flags |= FSCKED;\n \tif (obj->type == OBJ_BLOB)\n \t\treturn 0;\n \tif (obj->type == OBJ_TREE)\n@@ -524,37 +619,6 @@ static void fsck_dir(int i, char *path)\n \n static int default_refs;\n \n-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\tconst char *email, unsigned long timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n-{\n-\tstruct object *obj;\n-\n-\tif (verbose)\n-\t\tfprintf(stderr, \"Checking reflog %s->%s\\n\",\n-\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1));\n-\n-\tif (!is_null_sha1(osha1)) {\n-\t\tobj = lookup_object(osha1);\n-\t\tif (obj) {\n-\t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n-\t\t}\n-\t}\n-\tobj = lookup_object(nsha1);\n-\tif (obj) {\n-\t\tobj->used = 1;\n-\t\tmark_reachable(obj, REACHABLE);\n-\t}\n-\treturn 0;\n-}\n-\n-static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\tfor_each_reflog_ent(logname, fsck_handle_reflog_ent, NULL);\n-\treturn 0;\n-}\n-\n static int is_branch(const char *refname)\n {\n \treturn !strcmp(refname, \"HEAD\") || !prefixcmp(refname, \"refs/heads/\");\n@@ -574,7 +638,6 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f\n \t\terror(\"%s: not a commit\", refname);\n \tdefault_refs++;\n \tobj->used = 1;\n-\tmark_reachable(obj, REACHABLE);\n \n \treturn 0;\n }\n@@ -582,8 +645,6 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f\n static void get_default_heads(void)\n {\n \tfor_each_ref(fsck_handle_ref, NULL);\n-\tif (include_reflogs)\n-\t\tfor_each_reflog(fsck_handle_reflog, NULL);\n \n \t/*\n \t * Not having any default heads isn't really fatal, but\n@@ -660,7 +721,6 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\t sha1_to_hex(it->sha1));\n \t\t\treturn 1;\n \t\t}\n-\t\tmark_reachable(obj, REACHABLE);\n \t\tobj->used = 1;\n \t\tif (obj->type != OBJ_TREE)\n \t\t\terr |= objerror(obj, \"non-tree in cache-tree\");\n@@ -692,8 +752,8 @@ static struct option fsck_opts[] = {\n int cmd_fsck(int argc, const char **argv, const char *prefix)\n {\n \tint i, heads;\n+\tstruct rev_info revs;\n \n-\ttrack_object_refs = 1;\n \terrors_found = 0;\n \n \targc = parse_options(argc, argv, fsck_opts, fsck_usage, 0);\n@@ -731,6 +791,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \theads = 0;\n+\tinit_revisions(&revs, \"\");\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (!get_sha1(arg, head_sha1)) {\n@@ -740,8 +801,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\tif (!obj)\n \t\t\t\tcontinue;\n \n-\t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n+\t\t\tadd_pending_object(&revs, obj, arg);\n \t\t\theads++;\n \t\t\tcontinue;\n \t\t}\n@@ -760,25 +820,16 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \tif (keep_cache_objects) {\n \t\tread_cache();\n-\t\tfor (i = 0; i < active_nr; i++) {\n-\t\t\tunsigned int mode;\n-\t\t\tstruct blob *blob;\n-\t\t\tstruct object *obj;\n-\n-\t\t\tmode = active_cache[i]->ce_mode;\n-\t\t\tif (S_ISGITLINK(mode))\n-\t\t\t\tcontinue;\n-\t\t\tblob = lookup_blob(active_cache[i]->sha1);\n-\t\t\tif (!blob)\n-\t\t\t\tcontinue;\n-\t\t\tobj = &blob->object;\n-\t\t\tobj->used = 1;\n-\t\t\tmark_reachable(obj, REACHABLE);\n-\t\t}\n \t\tif (active_cache_tree)\n \t\t\tfsck_cache_tree(active_cache_tree);\n \t}\n \n+\tignore_reachability_errors = 1;\n+\tmark_reachable_objects(\n+\t\t&revs,\n+\t\t!heads,\n+\t\t!heads && include_reflogs,\n+\t\tkeep_cache_objects);\n \tcheck_connectivity();\n \treturn errors_found;\n }\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 692a761..2aaeb1a 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2013,7 +2013,6 @@ static void get_object_list(int ac, const char **av)\n \n \tinit_revisions(&revs, NULL);\n \tsave_commit_buffer = 0;\n-\ttrack_object_refs = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\ndiff --git a/builtin-prune.c b/builtin-prune.c\nindex bb8ead9..760e41d 100644\n--- a/builtin-prune.c\n+++ b/builtin-prune.c\n@@ -147,7 +147,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \n \tsave_commit_buffer = 0;\n \tinit_revisions(&revs, prefix);\n-\tmark_reachable_objects(&revs, 1);\n+\tmark_reachable_objects(&revs, 1, 1, 1);\n \n \tprune_object_dir(get_object_directory());\n \ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 4836ec9..108ca7a 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -374,7 +374,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tinit_revisions(&cb.revs, prefix);\n \t\tif (cb.verbose)\n \t\t\tprintf(\"Marking reachable objects...\");\n-\t\tmark_reachable_objects(&cb.revs, 0);\n+\t\tmark_reachable_objects(&cb.revs, 1, 0, 1);\n \t\tif (cb.verbose)\n \t\t\tputchar('\\n');\n \t}\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex de80158..14e86ce 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -605,7 +605,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tusage(rev_list_usage);\n \n \tsave_commit_buffer = revs.verbose_header || revs.grep_filter;\n-\ttrack_object_refs = 0;\n \tif (bisect_list)\n \t\trevs.limited = 1;\n \ndiff --git a/commit.c b/commit.c\nindex 8b8fb04..df4d331 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -290,17 +290,6 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t}\n \titem->date = parse_commit_date(bufptr, tail);\n \n-\tif (track_object_refs) {\n-\t\tunsigned i = 0;\n-\t\tstruct commit_list *p;\n-\t\tstruct object_refs *refs = alloc_object_refs(n_refs);\n-\t\tif (item->tree)\n-\t\t\trefs->ref[i++] = &item->tree->object;\n-\t\tfor (p = item->parents; p; p = p->next)\n-\t\t\trefs->ref[i++] = &p->item->object;\n-\t\tset_object_refs(&item->object, refs);\n-\t}\n-\n \treturn 0;\n }\n \ndiff --git a/object-refs.c b/object-refs.c\ndeleted file mode 100644\nindex 5345671..0000000\n--- a/object-refs.c\n+++ /dev/null\n@@ -1,87 +0,0 @@\n-#include \"cache.h\"\n-#include \"object.h\"\n-#include \"decorate.h\"\n-\n-int track_object_refs = 0;\n-\n-static struct decoration ref_decorate;\n-\n-struct object_refs *lookup_object_refs(struct object *base)\n-{\n-\treturn lookup_decoration(&ref_decorate, base);\n-}\n-\n-static void add_object_refs(struct object *obj, struct object_refs *refs)\n-{\n-\tif (add_decoration(&ref_decorate, obj, refs))\n-\t\tdie(\"object %s tried to add refs twice!\", sha1_to_hex(obj->sha1));\n-}\n-\n-struct object_refs *alloc_object_refs(unsigned count)\n-{\n-\tstruct object_refs *refs;\n-\tsize_t size = sizeof(*refs) + count*sizeof(struct object *);\n-\n-\trefs = xcalloc(1, size);\n-\trefs->count = count;\n-\treturn refs;\n-}\n-\n-static int compare_object_pointers(const void *a, const void *b)\n-{\n-\tconst struct object * const *pa = a;\n-\tconst struct object * const *pb = b;\n-\tif (*pa == *pb)\n-\t\treturn 0;\n-\telse if (*pa < *pb)\n-\t\treturn -1;\n-\telse\n-\t\treturn 1;\n-}\n-\n-void set_object_refs(struct object *obj, struct object_refs *refs)\n-{\n-\tunsigned int i, j;\n-\n-\t/* Do not install empty list of references */\n-\tif (refs->count < 1) {\n-\t\tfree(refs);\n-\t\treturn;\n-\t}\n-\n-\t/* Sort the list and filter out duplicates */\n-\tqsort(refs->ref, refs->count, sizeof(refs->ref[0]),\n-\t compare_object_pointers);\n-\tfor (i = j = 1; i < refs->count; i++) {\n-\t\tif (refs->ref[i] != refs->ref[i - 1])\n-\t\t\trefs->ref[j++] = refs->ref[i];\n-\t}\n-\tif (j < refs->count) {\n-\t\t/* Duplicates were found - reallocate list */\n-\t\tsize_t size = sizeof(*refs) + j*sizeof(struct object *);\n-\t\trefs->count = j;\n-\t\trefs = xrealloc(refs, size);\n-\t}\n-\n-\tfor (i = 0; i < refs->count; i++)\n-\t\trefs->ref[i]->used = 1;\n-\tadd_object_refs(obj, refs);\n-}\n-\n-void mark_reachable(struct object *obj, unsigned int mask)\n-{\n-\tconst struct object_refs *refs;\n-\n-\tif (!track_object_refs)\n-\t\tdie(\"cannot do reachability with object refs turned off\");\n-\t/* If we've been here already, don't bother */\n-\tif (obj->flags & mask)\n-\t\treturn;\n-\tobj->flags |= mask;\n-\trefs = lookup_object_refs(obj);\n-\tif (refs) {\n-\t\tunsigned i;\n-\t\tfor (i = 0; i < refs->count; i++)\n-\t\t\tmark_reachable(refs->ref[i], mask);\n-\t}\n-}\ndiff --git a/object.h b/object.h\nindex 397bbfa..20f560f 100644\n--- a/object.h\n+++ b/object.h\n@@ -6,11 +6,6 @@ struct object_list {\n \tstruct object_list *next;\n };\n \n-struct object_refs {\n-\tunsigned count;\n-\tstruct object *ref[FLEX_ARRAY]; /* more */\n-};\n-\n struct object_array {\n \tunsigned int nr;\n \tunsigned int alloc;\n@@ -35,14 +30,11 @@ struct object {\n \tunsigned char sha1[20];\n };\n \n-extern int track_object_refs;\n-\n extern const char *typename(unsigned int type);\n extern int type_from_string(const char *str);\n \n extern unsigned int get_max_object_index(void);\n extern struct object *get_indexed_object(unsigned int);\n-extern struct object_refs *lookup_object_refs(struct object *);\n \n /** Internal only **/\n struct object *lookup_object(const unsigned char *sha1);\n@@ -61,11 +53,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n /** Returns the object, with potentially excess memory allocated. **/\n struct object *lookup_unknown_object(const unsigned char *sha1);\n \n-struct object_refs *alloc_object_refs(unsigned count);\n-void set_object_refs(struct object *obj, struct object_refs *refs);\n-\n-void mark_reachable(struct object *obj, unsigned int mask);\n-\n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t struct object_list **list_p);\n \ndiff --git a/reachable.c b/reachable.c\nindex 00f289f..0ded91c 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -8,6 +8,8 @@\n #include \"reachable.h\"\n #include \"cache-tree.h\"\n \n+int ignore_reachability_errors;\n+\n static void process_blob(struct blob *blob,\n \t\t\t struct object_array *p,\n \t\t\t struct name_path *path,\n@@ -42,8 +44,20 @@ static void process_tree(struct tree *tree,\n \tif (obj->flags & SEEN)\n \t\treturn;\n \tobj->flags |= SEEN;\n-\tif (parse_tree(tree) < 0)\n-\t\tdie(\"bad tree object %s\", sha1_to_hex(obj->sha1));\n+\tif (!obj->parsed || !tree->buffer) {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\ttree->buffer = read_sha1_file(obj->sha1, &type, &size);\n+\t\tif (!tree->buffer || type != OBJ_TREE) {\n+\t\t\tif (!ignore_reachability_errors)\n+\t\t\t\tdie(\"bad tree object %s\", sha1_to_hex(obj->sha1));\n+\t\t\tfree(tree->buffer);\n+\t\t\ttree->buffer = NULL;\n+\t\t\treturn;\n+\t\t}\n+\t\ttree->size = size;\n+\t\tobj->parsed = 1;\n+\t}\n \tname = xstrdup(name);\n \tadd_object(obj, p, path, name);\n \tme.up = path;\n@@ -134,9 +148,10 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo\n \tstruct object *object = parse_object(sha1);\n \tstruct rev_info *revs = (struct rev_info *)cb_data;\n \n-\tif (!object)\n+\tif (object)\n+\t\tadd_pending_object(revs, object, \"\");\n+\telse if (!ignore_reachability_errors)\n \t\tdie(\"bad object ref: %s:%s\", path, sha1_to_hex(sha1));\n-\tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\n }\n@@ -166,6 +181,7 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs)\n static void add_cache_refs(struct rev_info *revs)\n {\n \tint i;\n+\tstruct blob *obj;\n \n \tread_cache();\n \tfor (i = 0; i < active_nr; i++) {\n@@ -179,7 +195,10 @@ static void add_cache_refs(struct rev_info *revs)\n \t\tif (S_ISGITLINK(active_cache[i]->ce_mode))\n \t\t\tcontinue;\n \n-\t\tlookup_blob(active_cache[i]->sha1);\n+\t\tobj = lookup_blob(active_cache[i]->sha1);\n+\t\tif (obj)\n+\t\t\tobj->object.flags |= SEEN;\n+\n \t\t/*\n \t\t * We could add the blobs to the pending list, but quite\n \t\t * frankly, we don't care. Once we've looked them up, and\n@@ -191,7 +210,11 @@ static void add_cache_refs(struct rev_info *revs)\n \t\tadd_cache_tree(active_cache_tree, revs);\n }\n \n-void mark_reachable_objects(struct rev_info *revs, int mark_reflog)\n+void mark_reachable_objects(\n+\t\tstruct rev_info *revs,\n+\t\tint mark_refs,\n+\t\tint mark_reflog,\n+\t\tint mark_cache)\n {\n \t/*\n \t * Set up revision parsing, and mark us as being interested\n@@ -202,13 +225,15 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog)\n \trevs->tree_objects = 1;\n \n \t/* Add all refs from the index file */\n-\tadd_cache_refs(revs);\n+\tif (mark_cache)\n+\t\tadd_cache_refs(revs);\n \n \t/* Add all external refs */\n-\tfor_each_ref(add_one_ref, revs);\n+\tif (mark_refs)\n+\t\tfor_each_ref(add_one_ref, revs);\n \n \t/* Add all reflog info */\n-\tif (mark_reflog)\n+\tif (mark_refs && mark_reflog)\n \t\tfor_each_reflog(add_one_reflog, revs);\n \n \t/*\ndiff --git a/reachable.h b/reachable.h\nindex 4075181..802d464 100644\n--- a/reachable.h\n+++ b/reachable.h\n@@ -1,6 +1,11 @@\n #ifndef REACHEABLE_H\n #define REACHEABLE_H\n \n-extern void mark_reachable_objects(struct rev_info *revs, int mark_reflog);\n+extern int ignore_reachability_errors;\n+extern void mark_reachable_objects(\n+\t\tstruct rev_info *revs,\n+\t\tint mark_refs,\n+\t\tint mark_reflog,\n+\t\tint mark_cache);\n \n #endif\ndiff --git a/tag.c b/tag.c\nindex 38bf913..d19f56d 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -84,12 +84,6 @@ int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n \t\titem->tagged = NULL;\n \t}\n \n-\tif (item->tagged && track_object_refs) {\n-\t\tstruct object_refs *refs = alloc_object_refs(1);\n-\t\trefs->ref[0] = item->tagged;\n-\t\tset_object_refs(&item->object, refs);\n-\t}\n-\n \treturn 0;\n }\n \ndiff --git a/tree.c b/tree.c\nindex 87708ef..4b1825c 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -202,52 +202,6 @@ struct tree *lookup_tree(const unsigned char *sha1)\n \treturn (struct tree *) obj;\n }\n \n-/*\n- * NOTE! Tree refs to external git repositories\n- * (ie gitlinks) do not count as real references.\n- *\n- * You don't have to have those repositories\n- * available at all, much less have the objects\n- * accessible from the current repository.\n- */\n-static void track_tree_refs(struct tree *item)\n-{\n-\tint n_refs = 0, i;\n-\tstruct object_refs *refs;\n-\tstruct tree_desc desc;\n-\tstruct name_entry entry;\n-\n-\t/* Count how many entries there are.. */\n-\tinit_tree_desc(&desc, item->buffer, item->size);\n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tif (S_ISGITLINK(entry.mode))\n-\t\t\tcontinue;\n-\t\tn_refs++;\n-\t}\n-\n-\t/* Allocate object refs and walk it again.. */\n-\ti = 0;\n-\trefs = alloc_object_refs(n_refs);\n-\tinit_tree_desc(&desc, item->buffer, item->size);\n-\twhile (tree_entry(&desc, &entry)) {\n-\t\tstruct object *obj;\n-\n-\t\tif (S_ISGITLINK(entry.mode))\n-\t\t\tcontinue;\n-\t\tif (S_ISDIR(entry.mode))\n-\t\t\tobj = &lookup_tree(entry.sha1)->object;\n-\t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode))\n-\t\t\tobj = &lookup_blob(entry.sha1)->object;\n-\t\telse {\n-\t\t\twarning(\"in tree %s: entry %s has bad mode %.6o\\n\",\n-\t\t\t sha1_to_hex(item->object.sha1), entry.path, entry.mode);\n-\t\t\tobj = lookup_unknown_object(entry.sha1);\n-\t\t}\n-\t\trefs->ref[i++] = obj;\n-\t}\n-\tset_object_refs(&item->object, refs);\n-}\n-\n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n {\n \tif (item->object.parsed)\n@@ -256,8 +210,6 @@ int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\n \titem->buffer = buffer;\n \titem->size = size;\n \n-\tif (track_object_refs)\n-\t\ttrack_tree_refs(item);\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 51e3ec4..7c2f5e5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -392,7 +392,6 @@ static int get_common_commits(void)\n \tchar hex[41], last_hex[41];\n \tint len;\n \n-\ttrack_object_refs = 0;\n \tsave_commit_buffer = 0;\n \n \tfor(;;) {\ndiff --git a/walker.c b/walker.c\nindex adc3e80..c10eca8 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -256,7 +256,6 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tint i;\n \n \tsave_commit_buffer = 0;\n-\ttrack_object_refs = 0;\n \n \tfor (i = 0; i < targets; i++) {\n \t\tif (!write_ref || !write_ref[i])\n\n-- \nShawn.\n"},{"id":"68762","messageId":"20080214190455.GB26527@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LSU.1.00.0802131200410.30505@racer.site","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-14T19:04:55Z","receivedAt":"2008-02-14T19:04:55Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Wed, Feb 13, 2008 at 12:01:43PM +0000, Johannes Schindelin wrote:\n> On Wed, 13 Feb 2008, Martin Koegler wrote:\n> > This would mean, that we must make git-rev-list and git-pack-objects not \n> > segfault on incorrect links between objects.\n> \n> We should do that anyway. It may error out, but segfaulting is no option.\n> \n> So if you have a test case, please make it public so we can fix the \n> breakage.\n\nOK, here is the first part of missing checks. \n\nIts still WIP:\n* It should be checked, if reactions to errors are correct.\n* Patch should be splitted and turned into submitable pieces\n\nIf anybody wants to do this, please go on.\n\n>From 4a6908ff51059a5b2bcc8d8c41fd41de53ee9151 Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Thu, 14 Feb 2008 19:34:39 +0100\nSubject: [PATCH] Missing checks (1)\n\nSigned-off-by: Martin Koegler \n---\n commit.c | 16 +++++++++++-----\n revision.c | 12 ++++++++++++\n sha1_file.c | 3 ++-\n sha1_name.c | 11 ++++++++++-\n tag.c | 5 ++++-\n 5 files changed, 39 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 8b8fb04..53f0042 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -311,6 +311,8 @@ int parse_commit(struct commit *item)\n \tunsigned long size;\n \tint ret;\n \n+\tif (!item)\n+\t\treturn -1;\n \tif (item->object.parsed)\n \t\treturn 0;\n \tbuffer = read_sha1_file(item->object.sha1, &type, &size);\n@@ -385,8 +387,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,\n \n \twhile (parents) {\n \t\tstruct commit *commit = parents->item;\n-\t\tparse_commit(commit);\n-\t\tif (!(commit->object.flags & mark)) {\n+\t\tif (!parse_commit(commit) && !(commit->object.flags & mark)) {\n \t\t\tcommit->object.flags |= mark;\n \t\t\tinsert_by_date(commit, list);\n \t\t}\n@@ -552,8 +553,10 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)\n \t\t */\n \t\treturn commit_list_insert(one, &result);\n \n-\tparse_commit(one);\n-\tparse_commit(two);\n+\tif (parse_commit(one))\n+\t\tdie(\"invalid commit\");\n+\tif (parse_commit(two))\n+\t\tdie(\"invalid commit\");\n \n \tone->object.flags |= PARENT1;\n \ttwo->object.flags |= PARENT2;\n@@ -584,9 +587,12 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)\n \t\twhile (parents) {\n \t\t\tstruct commit *p = parents->item;\n \t\t\tparents = parents->next;\n+\t\t\tif (!p)\n+\t\t\t\tdie(\"invalid commit\");\n \t\t\tif ((p->object.flags & flags) == flags)\n \t\t\t\tcontinue;\n-\t\t\tparse_commit(p);\n+\t\t\tif(parse_commit(p))\n+\t\t\t\tdie(\"invalid commit\");\n \t\t\tp->object.flags |= flags;\n \t\t\tinsert_by_date(p, &list);\n \t\t}\ndiff --git a/revision.c b/revision.c\nindex 6e85aaa..9f8723d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -46,6 +46,8 @@ void add_object(struct object *obj,\n \n static void mark_blob_uninteresting(struct blob *blob)\n {\n+\tif (!blob)\n+\t\treturn;\n \tif (blob->object.flags & UNINTERESTING)\n \t\treturn;\n \tblob->object.flags |= UNINTERESTING;\n@@ -57,6 +59,8 @@ void mark_tree_uninteresting(struct tree *tree)\n \tstruct name_entry entry;\n \tstruct object *obj = &tree->object;\n \n+\tif (!obj)\n+\t\treturn;\n \tif (obj->flags & UNINTERESTING)\n \t\treturn;\n \tobj->flags |= UNINTERESTING;\n@@ -94,6 +98,8 @@ void mark_parents_uninteresting(struct commit *commit)\n \n \twhile (parents) {\n \t\tstruct commit *commit = parents->item;\n+\t\tif (!commit)\n+\t\t\tcontinue;\n \t\tif (!(commit->object.flags & UNINTERESTING)) {\n \t\t\tcommit->object.flags |= UNINTERESTING;\n \n@@ -173,6 +179,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\tstruct tag *tag = (struct tag *) object;\n \t\tif (revs->tag_objects && !(flags & UNINTERESTING))\n \t\t\tadd_pending_object(revs, object, tag->tag);\n+\t\tif (!tag->tagged)\n+\t\t\tdie(\"bad tag\");\n \t\tobject = parse_object(tag->tagged->sha1);\n \t\tif (!object)\n \t\t\tdie(\"bad object %s\", sha1_to_hex(tag->tagged->sha1));\n@@ -685,12 +693,16 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)\n \t\tit = get_reference(revs, arg, sha1, 0);\n \t\tif (it->type != OBJ_TAG)\n \t\t\tbreak;\n+\t\tif (!((struct tag*)it)->tagged)\n+\t\t\treturn 0;\n \t\thashcpy(sha1, ((struct tag*)it)->tagged->sha1);\n \t}\n \tif (it->type != OBJ_COMMIT)\n \t\treturn 0;\n \tcommit = (struct commit *)it;\n \tfor (parents = commit->parents; parents; parents = parents->next) {\n+\t\tif (!parents->item)\n+\t\t\tcontinue;\n \t\tit = &parents->item->object;\n \t\tit->flags |= flags;\n \t\tadd_pending_object(revs, it, arg);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4179949..c25ce64 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1943,7 +1943,8 @@ void *read_object_with_reference(const unsigned char *sha1,\n \t\t}\n \t\tref_length = strlen(ref_type);\n \n-\t\tif (memcmp(buffer, ref_type, ref_length) ||\n+\t\tif (ref_length + 40 >= size ||\n+\t\t memcmp(buffer, ref_type, ref_length) ||\n \t\t get_sha1_hex((char *) buffer + ref_length, actual_sha1)) {\n \t\t\tfree(buffer);\n \t\t\treturn NULL;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex be8489e..bd0bce6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -396,6 +396,8 @@ static int get_parent(const char *name, int len,\n \tp = commit->parents;\n \twhile (p) {\n \t\tif (!--idx) {\n+\t\t\tif (!p->item)\n+\t\t\t\treturn -1;\n \t\t\thashcpy(result, p->item->object.sha1);\n \t\t\treturn 0;\n \t\t}\n@@ -417,6 +419,8 @@ static int get_nth_ancestor(const char *name, int len,\n \n \t\tif (!commit || parse_commit(commit) || !commit->parents)\n \t\t\treturn -1;\n+\t\tif (!commit->parents->item)\n+\t\t\treturn -1;\n \t\thashcpy(sha1, commit->parents->item->object.sha1);\n \t}\n \thashcpy(result, sha1);\n@@ -494,6 +498,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n \t\t\t\treturn error(\"%.*s: expected %s type, but the object dereferences to %s type\",\n \t\t\t\t\t len, name, typename(expected_type),\n \t\t\t\t\t typename(o->type));\n+\t\t\tif (!o)\n+\t\t\t\treturn -1;\n \t\t\tif (!o->parsed)\n \t\t\t\tparse_object(o->sha1);\n \t\t}\n@@ -580,6 +586,8 @@ static int handle_one_ref(const char *path,\n \t\treturn 0;\n \tif (object->type == OBJ_TAG)\n \t\tobject = deref_tag(object, path, strlen(path));\n+\tif (!object)\n+\t\treturn 0;\n \tif (object->type != OBJ_COMMIT)\n \t\treturn 0;\n \tinsert_by_date((struct commit *)object, list);\n@@ -617,7 +625,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \t\tunsigned long size;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n-\t\tparse_object(commit->object.sha1);\n+\t\tif(!parse_object(commit->object.sha1))\n+\t\t\tcontinue;\n \t\tif (temp_commit_buffer)\n \t\t\tfree(temp_commit_buffer);\n \t\tif (commit->buffer)\ndiff --git a/tag.c b/tag.c\nindex 38bf913..990134f 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -9,7 +9,10 @@ const char *tag_type = \"tag\";\n struct object *deref_tag(struct object *o, const char *warn, int warnlen)\n {\n \twhile (o && o->type == OBJ_TAG)\n-\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n+\t\tif (((struct tag *)o)->tagged)\n+\t\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n+\t\telse\n+\t\t\to = NULL;\n \tif (!o && warn) {\n \t\tif (!warnlen)\n \t\t\twarnlen = strlen(warn);\n-- \n1.5.4.1.gb43a\n"},{"id":"68763","messageId":"20080214190746.GC26527@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"20080214090013.GK24004@spearce.org","subject":"Re: [RFC PATCH] Remove object-refs from fsck","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-14T19:07:47Z","receivedAt":"2008-02-14T19:07:47Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Thu, Feb 14, 2008 at 04:00:13AM -0500, Shawn O. Pearce wrote:\n> \n> +static int object_exists(const unsigned char *sha1)\n> +{\n> +\tstruct object *obj = lookup_object(sha1);\n> +\tif (obj && obj->flags & FSCKED)\n> +\t\treturn 1;\n> +\treturn has_sha1_file(sha1);\n> +}\n> +\n> +static void broken_link(struct object *obj,\n> +\t\tenum object_type ref_type,\n> +\t\tconst unsigned char *ref_sha1)\n> +{\n> +\tprintf(\"broken link from %7s %s\\n\",\n> +\t typename(obj->type), sha1_to_hex(obj->sha1));\n> +\tprintf(\" to %7s %s\\n\",\n> +\t typename(ref_type), sha1_to_hex(ref_sha1));\n> +\terrors_found |= ERROR_REACHABLE;\n> +}\n> +\n> +static void broken_tree(struct tree *item)\n> +{\n> +\tstruct tree_desc desc;\n> +\n> +\titem->object.parsed = 0;\n> +\tif (parse_tree(item) < 0)\n> +\t\treturn; /* error already displayed */\n> +\n> +\tinit_tree_desc(&desc, item->buffer, item->size);\n> +\twhile (desc.size) {\n> +\t\tunsigned mode;\n> +\t\tconst char *name;\n> +\t\tconst unsigned char *sha1;\n> +\n> +\t\tsha1 = tree_entry_extract(&desc, &name, &mode);\n> +\t\tupdate_tree_entry(&desc);\n> +\t\tif (S_ISGITLINK(mode) || object_exists(sha1))\n> +\t\t\tcontinue;\n> +\t\tif (S_ISDIR(mode))\n> +\t\t\tbroken_link(&item->object, OBJ_TREE, sha1);\n> +\t\telse\n> +\t\t\tbroken_link(&item->object, OBJ_BLOB, sha1);\n> +\t}\n> +\tfree(item->buffer);\n> +\titem->buffer = NULL;\n> +}\n> +\n> +static void broken_commit(struct commit *commit)\n> +{\n> +\tstruct tree *tree = commit->tree;\n> +\tstruct commit_list *pp;\n> +\n> +\tif (tree && !object_exists(tree->object.sha1))\n> +\t\tbroken_link(&commit->object, OBJ_TREE, tree->object.sha1);\n> +\n> +\tpp = commit->parents;\n> +\twhile (pp) {\n> +\t\tstruct commit *p = pp->item;\n> +\t\tif (p && !object_exists(p->object.sha1))\n> +\t\t\tbroken_link(&commit->object, OBJ_COMMIT, p->object.sha1);\n> +\t\tpp = pp->next;\n> +\t}\n> +}\n> +\n> +static void broken_tag(struct tag *tag)\n> +{\n> +\tstruct object *ref = tag->tagged;\n> +\tif (ref && !object_exists(ref->sha1))\n> +\t\tbroken_link(&tag->object, ref->type, ref->sha1);\n> +}\n> +\n> /*\n> * Check a single reachable object\n> */\n> static void check_reachable_object(struct object *obj)\n> {\n> -\tconst struct object_refs *refs;\n> -\n> \t/*\n> \t * We obviously want the object to be parsed,\n> \t * except if it was in a pack-file and we didn't\n> @@ -84,21 +155,28 @@ static void check_reachable_object(struct object *obj)\n> \t}\n> \n> \t/*\n> -\t * Check that everything that we try to reference is also good.\n> +\t * If the fsck routines found this object WANTING\n> +\t * then we need to re-evaluate what it wanted and\n> +\t * report on those broken links.\n> \t */\n> -\trefs = lookup_object_refs(obj);\n> -\tif (refs) {\n> -\t\tunsigned j;\n> -\t\tfor (j = 0; j < refs->count; j++) {\n> -\t\t\tstruct object *ref = refs->ref[j];\n> -\t\t\tif (ref->parsed ||\n> -\t\t\t (has_sha1_file(ref->sha1)))\n> -\t\t\t\tcontinue;\n> -\t\t\tprintf(\"broken link from %7s %s\\n\",\n> -\t\t\t typename(obj->type), sha1_to_hex(obj->sha1));\n> -\t\t\tprintf(\" to %7s %s\\n\",\n> -\t\t\t typename(ref->type), sha1_to_hex(ref->sha1));\n> -\t\t\terrors_found |= ERROR_REACHABLE;\n> +\tif (obj->flags & WANTING) {\n> +\t\tswitch (obj->type) {\n> +\t\tcase OBJ_TREE:\n> +\t\t\tbroken_tree((struct tree *) obj);\n> +\t\t\tbreak;\n> +\n> +\t\tcase OBJ_COMMIT:\n> +\t\t\tbroken_commit((struct commit *) obj);\n> +\t\t\tbreak;\n> +\n> +\t\tcase OBJ_TAG:\n> +\t\t\tbroken_tag((struct tag *) obj);\n> +\t\t\tbreak;\n> +\n> +\t\tdefault:\n> +\t\t\tobjerror(obj,\n> +\t\t\t\t\"type '%d' has broken link (internal fsck error)\",\n> +\t\t\t\tobj->type);\n> \t\t}\n> \t}\n> }\n> @@ -181,7 +259,7 @@ static void check_object(struct object *obj)\n> \tif (verbose)\n> \t\tfprintf(stderr, \"Checking %s\\n\", sha1_to_hex(obj->sha1));\n> \n> -\tif (obj->flags & REACHABLE)\n> +\tif (obj->flags & SEEN)\n> \t\tcheck_reachable_object(obj);\n> \telse\n> \t\tcheck_unreachable_object(obj);\n> @@ -309,6 +387,15 @@ static int fsck_tree(struct tree *item)\n> \t\t\thas_bad_modes = 1;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * If the SHA-1 should exist in this repository but\n> +\t\t * we are missing it flag this tree as WANTING. We\n> +\t\t * will revisit this error later once we determine\n> +\t\t * this tree is SEEN.\n> +\t\t */\n> +\t\tif (!S_ISGITLINK(mode) && !object_exists(sha1))\n> +\t\t\titem->object.flags |= WANTING;\n> +\n> \t\tif (o_name) {\n> \t\t\tswitch (verify_ordered(o_mode, o_name, mode, name)) {\n> \t\t\tcase TREE_UNORDERED:\n> @@ -367,10 +454,15 @@ static int fsck_commit(struct commit *commit)\n> \t\treturn objerror(&commit->object, \"invalid format - expected 'tree' line\");\n> \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n> \t\treturn objerror(&commit->object, \"invalid 'tree' line format - bad sha1\");\n> +\tif (!object_exists(tree_sha1))\n> +\t\tcommit->object.flags |= WANTING;\n> +\n\nHow do we verify, that tree_sha1 is a tree?\n\n\n> \tbuffer += 46;\n> \twhile (!memcmp(buffer, \"parent \", 7)) {\n> \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n> \t\t\treturn objerror(&commit->object, \"invalid 'parent' line format - bad sha1\");\n> +\t\tif (!object_exists(sha1))\n> +\t\t\tcommit->object.flags |= WANTING;\n\nHow do we verify, that sha1 is a commit?\n\n> \t\tbuffer += 48;\n> \t}\n> \tif (memcmp(buffer, \"author \", 7))\n> @@ -395,6 +487,9 @@ static int fsck_tag(struct tag *tag)\n> \tif (!tagged) {\n> \t\treturn objerror(&tag->object, \"could not load tagged object\");\n> \t}\n> +\tif (!object_exists(tagged->sha1))\n> +\t\ttag->object.flags |= WANTING;\n> +\n\nSame here. Where do we check the type?\n\nmfg Martin Kögler\n"},{"id":"68788","messageId":"alpine.LSU.1.00.0802142345450.30505@racer.site","threadId":"12011","inReplyTo":"20080214190455.GB26527@auto.tuwien.ac.at","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-15T00:06:49Z","receivedAt":"2008-02-15T00:06:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Feb 2008, Martin Koegler wrote:\n\n> diff --git a/commit.c b/commit.c\n> index 8b8fb04..53f0042 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -311,6 +311,8 @@ int parse_commit(struct commit *item)\n> \tunsigned long size;\n> \tint ret;\n> \n> +\tif (!item)\n> +\t\treturn -1;\n\nYeah, I just grepped for users of parse_commit(), and there are just too \nmany, so this change makes sense.\n\n> @@ -385,8 +387,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,\n> \n> \twhile (parents) {\n> \t\tstruct commit *commit = parents->item;\n> -\t\tparse_commit(commit);\n> -\t\tif (!(commit->object.flags & mark)) {\n> +\t\tif (!parse_commit(commit) && !(commit->object.flags & mark)) {\n\nThis makes sense, too.\n\n> @@ -552,8 +553,10 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)\n> \t\t */\n> \t\treturn commit_list_insert(one, &result);\n> \n> -\tparse_commit(one);\n> -\tparse_commit(two);\n> +\tif (parse_commit(one))\n> +\t\tdie(\"invalid commit\");\n> +\tif (parse_commit(two))\n> +\t\tdie(\"invalid commit\");\n\nI'd rather have this return NULL after displaying an error. After all, \nmerge_bases() is sort of \"libified\".\n\nBTW a simple \"git grep parse_commit\\( | grep -vwe if -e gitweb\" shows \nthis:\n\nbuiltin-blame.c: parse_commit(commit);\nbuiltin-checkout.c: parse_commit(commit);\nbuiltin-checkout.c: parse_commit(old->commit);\nbuiltin-checkout.c: parse_commit(new->commit);\nbuiltin-checkout.c: parse_commit(new.commit);\nbuiltin-describe.c: parse_commit(p);\nbuiltin-describe.c: parse_commit(p);\nbuiltin-fast-export.c: parse_commit(commit);\nbuiltin-fast-export.c: parse_commit(commit->parents->item);\nbuiltin-fetch-pack.c: parse_commit(commit);\nbuiltin-fetch-pack.c: parse_commit(commit);\nbuiltin-fetch-pack.c: parse_commit(commit);\nbuiltin-name-rev.c: parse_commit(commit);\nbuiltin-show-branch.c: parse_commit(p);\nbuiltin-show-branch.c: parse_commit(commit);\ncommit.c:int parse_commit(struct commit *item)\ncommit.c: parse_commit(commit);\ncommit.c: parse_commit(one);\ncommit.c: parse_commit(two);\ncommit.c: parse_commit(p);\ncommit.h:int parse_commit(struct commit *item);\ncontrib/gitview/gitview: self.parse_commit(commit_lines)\ncontrib/gitview/gitview: def parse_commit(self, commit_lines):\nshallow.c: parse_commit(commit);\nupload-pack.c: parse_commit((struct commit *)object);\n\nA few of those need checking, too, I think.\n\n> @@ -584,9 +587,12 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)\n> \t\twhile (parents) {\n> \t\t\tstruct commit *p = parents->item;\n> \t\t\tparents = parents->next;\n> +\t\t\tif (!p)\n> +\t\t\t\tdie(\"invalid commit\");\n> \t\t\tif ((p->object.flags & flags) == flags)\n> \t\t\t\tcontinue;\n> -\t\t\tparse_commit(p);\n> +\t\t\tif(parse_commit(p))\n> +\t\t\t\tdie(\"invalid commit\");\n> \t\t\tp->object.flags |= flags;\n> \t\t\tinsert_by_date(p, &list);\n> \t\t}\n\nAgain, please do not die() in merge_bases().\n\n> diff --git a/revision.c b/revision.c\n> index 6e85aaa..9f8723d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -46,6 +46,8 @@ void add_object(struct object *obj,\n> \n> static void mark_blob_uninteresting(struct blob *blob)\n> {\n> +\tif (!blob)\n> +\t\treturn;\n\nIMHO not needed [*1*]. The first user of this (static) function calls it \nwith lookup_blob(), which assumes that the blob has been read already.\n\nThe second uses a checked object.\n\n> @@ -57,6 +59,8 @@ void mark_tree_uninteresting(struct tree *tree)\n> \tstruct name_entry entry;\n> \tstruct object *obj = &tree->object;\n> \n> +\tif (!obj)\n> +\t\treturn;\n\nSame here.\n\n> @@ -94,6 +98,8 @@ void mark_parents_uninteresting(struct commit *commit)\n> \n> \twhile (parents) {\n> \t\tstruct commit *commit = parents->item;\n> +\t\tif (!commit)\n> +\t\t\tcontinue;\n\nCan parents->item really be NULL? I think not. And indeed, a little \nsearch reveals that parse_commit_buffer() does this when it constructs the \nparents list, and encounters an invalid SHA-1:\n\nreturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n\nIn case it is a valid SHA-1, but not a commit, it is ignored.\n\n> @@ -173,6 +179,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n> \t\tstruct tag *tag = (struct tag *) object;\n> \t\tif (revs->tag_objects && !(flags & UNINTERESTING))\n> \t\t\tadd_pending_object(revs, object, tag->tag);\n> +\t\tif (!tag->tagged)\n> +\t\t\tdie(\"bad tag\");\n\nI haven't looked yet, but I suspect that this is as impossible as the \ninvalid parent.\n\n> @@ -685,12 +693,16 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)\n> \t\tit = get_reference(revs, arg, sha1, 0);\n> \t\tif (it->type != OBJ_TAG)\n> \t\t\tbreak;\n> +\t\tif (!((struct tag*)it)->tagged)\n> +\t\t\treturn 0;\n\nSee above.\n\n> \t\thashcpy(sha1, ((struct tag*)it)->tagged->sha1);\n> \t}\n> \tif (it->type != OBJ_COMMIT)\n> \t\treturn 0;\n> \tcommit = (struct commit *)it;\n> \tfor (parents = commit->parents; parents; parents = parents->next) {\n> +\t\tif (!parents->item)\n> +\t\t\tcontinue;\n\nSee above.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 4179949..c25ce64 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1943,7 +1943,8 @@ void *read_object_with_reference(const unsigned char *sha1,\n> \t\t}\n> \t\tref_length = strlen(ref_type);\n> \n> -\t\tif (memcmp(buffer, ref_type, ref_length) ||\n> +\t\tif (ref_length + 40 >= size ||\n> +\t\t memcmp(buffer, ref_type, ref_length) ||\n\nMakes sense.\n\n> diff --git a/sha1_name.c b/sha1_name.c\n> index be8489e..bd0bce6 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -396,6 +396,8 @@ static int get_parent(const char *name, int len,\n> \tp = commit->parents;\n> \twhile (p) {\n> \t\tif (!--idx) {\n> +\t\t\tif (!p->item)\n> +\t\t\t\treturn -1;\n\nSee above.\n\n> @@ -417,6 +419,8 @@ static int get_nth_ancestor(const char *name, int len,\n> \n> \t\tif (!commit || parse_commit(commit) || !commit->parents)\n> \t\t\treturn -1;\n> +\t\tif (!commit->parents->item)\n> +\t\t\treturn -1;\n\nSee above.\n\n> @@ -494,6 +498,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n> \t\t\t\treturn error(\"%.*s: expected %s type, but the object dereferences to %s type\",\n> \t\t\t\t\t len, name, typename(expected_type),\n> \t\t\t\t\t typename(o->type));\n> +\t\t\tif (!o)\n> +\t\t\t\treturn -1;\n\nYou probably want to guard for ((tag *)o)->tagged == NULL; Okay, now I am \ncurious. *megoesandlooks* Indeed, if the tag refers to an unknown type, \ntagged = NULL.\n\nBut I think in that case, it should return an error(). And maybe only for \ntype == OBJ_TAG.\n\n> @@ -580,6 +586,8 @@ static int handle_one_ref(const char *path,\n> \t\treturn 0;\n> \tif (object->type == OBJ_TAG)\n> \t\tobject = deref_tag(object, path, strlen(path));\n> +\tif (!object)\n> +\t\treturn 0;\n\nAs above, it looks strange to see that object->something is checked, and \n_then_ object. I'd put this into curly brackets, together with the \nderef_tag(), just to help the reader a bit, should she be as weak in mind \nas yours truly.\n\n> @@ -617,7 +625,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n> \t\tunsigned long size;\n> \n> \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n> -\t\tparse_object(commit->object.sha1);\n> +\t\tif(!parse_object(commit->object.sha1))\n> +\t\t\tcontinue;\n\nMakes sense, but please add a space after the \"if\".\n\n> diff --git a/tag.c b/tag.c\n> index 38bf913..990134f 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -9,7 +9,10 @@ const char *tag_type = \"tag\";\n> struct object *deref_tag(struct object *o, const char *warn, int warnlen)\n> {\n> \twhile (o && o->type == OBJ_TAG)\n> -\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n> +\t\tif (((struct tag *)o)->tagged)\n> +\t\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n> +\t\telse\n> +\t\t\to = NULL;\n\nKnowing that tagged _can_ be NULL, this makes tons of sense. Except that \nagain, I'd call error() to tell the user, before setting o = NULL.\n\nCiao,\nDscho\n\n[*1*] There might be a few people arguing that defensive programming is \nnever wrong.\n\nAlas, I saw my share of defensive programming, and more often than not, \nthe real bugs were hard to find in between all those checks. And some \nchecks were actively wrong -- again hard to see...\n\nRemember: You can make code so simple that there are obviously no bugs. \nAnd the other way is to make it so complicated that there are no obvious \nbugs.\n"},{"id":"68798","messageId":"20080215071800.GA27772@auto.tuwien.ac.at","threadId":"12011","inReplyTo":"alpine.LSU.1.00.0802142345450.30505@racer.site","subject":"Re: [RFC Patch] Preventing corrupt objects from entering the repository","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-15T07:18:00Z","receivedAt":"2008-02-15T07:18:00Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Fri, Feb 15, 2008 at 12:06:49AM +0000, Johannes Schindelin wrote:\n> On Thu, 14 Feb 2008, Martin Koegler wrote:\n> > @@ -552,8 +553,10 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)\n> > \t\t */\n> > \t\treturn commit_list_insert(one, &result);\n> > \n> > -\tparse_commit(one);\n> > -\tparse_commit(two);\n> > +\tif (parse_commit(one))\n> > +\t\tdie(\"invalid commit\");\n> > +\tif (parse_commit(two))\n> > +\t\tdie(\"invalid commit\");\n> \n> I'd rather have this return NULL after displaying an error. After all, \n> merge_bases() is sort of \"libified\".\n\nWould all caller of get_merge_bases treat NULL as an error?\n\n> BTW a simple \"git grep parse_commit\\( | grep -vwe if -e gitweb\" shows \n> this:\n> \n> builtin-blame.c: parse_commit(commit);\n> builtin-checkout.c: parse_commit(commit);\n> builtin-checkout.c: parse_commit(old->commit);\n> builtin-checkout.c: parse_commit(new->commit);\n> builtin-checkout.c: parse_commit(new.commit);\n> builtin-describe.c: parse_commit(p);\n> builtin-describe.c: parse_commit(p);\n> builtin-fast-export.c: parse_commit(commit);\n> builtin-fast-export.c: parse_commit(commit->parents->item);\n> builtin-fetch-pack.c: parse_commit(commit);\n> builtin-fetch-pack.c: parse_commit(commit);\n> builtin-fetch-pack.c: parse_commit(commit);\n> builtin-name-rev.c: parse_commit(commit);\n> builtin-show-branch.c: parse_commit(p);\n> builtin-show-branch.c: parse_commit(commit);\n> commit.c:int parse_commit(struct commit *item)\n> commit.c: parse_commit(commit);\n> commit.c: parse_commit(one);\n> commit.c: parse_commit(two);\n> commit.c: parse_commit(p);\n> commit.h:int parse_commit(struct commit *item);\n> contrib/gitview/gitview: self.parse_commit(commit_lines)\n> contrib/gitview/gitview: def parse_commit(self, commit_lines):\n> shallow.c: parse_commit(commit);\n> upload-pack.c: parse_commit((struct commit *)object);\n> \n> A few of those need checking, too, I think.\n\nSame for prepare_revision_walk. It can fail because an error in parse_XXX,\nbut no caller checks the result.\n\n \n> > diff --git a/revision.c b/revision.c\n> > index 6e85aaa..9f8723d 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -46,6 +46,8 @@ void add_object(struct object *obj,\n> > \n> > static void mark_blob_uninteresting(struct blob *blob)\n> > {\n> > +\tif (!blob)\n> > +\t\treturn;\n> \n> IMHO not needed [*1*]. The first user of this (static) function calls it \n> with lookup_blob(), which assumes that the blob has been read already.\n\nWhat about calling handle_revision_arg with \"^\", where a\nnon blob object is stored under a blob mode?\n\nhandle_revision_arg will parse only the tree and put it on the pending\nlist. prepare_revision_walk will call mark_tree_uninteresting on it.\nFor any object with blob mode in the tree, it calls\nmark_blob_uninteresting(lookup_blob(entry.sha1)); If entry.sha1 is the\nsha1 of an already loaded non blob object, we need this check.\n\n> [*1*] There might be a few people arguing that defensive programming is \n> never wrong.\n> \n> Alas, I saw my share of defensive programming, and more often than not, \n> the real bugs were hard to find in between all those checks. And some \n> checks were actively wrong -- again hard to see...\n> \n> Remember: You can make code so simple that there are obviously no bugs. \n> And the other way is to make it so complicated that there are no obvious \n> bugs.\n\nI would put rev_info in the second category. What can not be done with\nit?\n\n> > @@ -57,6 +59,8 @@ void mark_tree_uninteresting(struct tree *tree)\n> > \tstruct name_entry entry;\n> > \tstruct object *obj = &tree->object;\n> > \n> > +\tif (!obj)\n> > +\t\treturn;\n> \n> Same here.\n\nSame argument (s/blob/tree/g).\n\n> > @@ -94,6 +98,8 @@ void mark_parents_uninteresting(struct commit *commit)\n> > \n> > \twhile (parents) {\n> > \t\tstruct commit *commit = parents->item;\n> > +\t\tif (!commit)\n> > +\t\t\tcontinue;\n> \n> Can parents->item really be NULL? I think not. And indeed, a little \n> search reveals that parse_commit_buffer() does this when it constructs the \n> parents list, and encounters an invalid SHA-1:\n> \n> return error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n> \n> In case it is a valid SHA-1, but not a commit, it is ignored.\n\nIts unnecessary, I missed this check.\n\n> > @@ -173,6 +179,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n> > \t\tstruct tag *tag = (struct tag *) object;\n> > \t\tif (revs->tag_objects && !(flags & UNINTERESTING))\n> > \t\t\tadd_pending_object(revs, object, tag->tag);\n> > +\t\tif (!tag->tagged)\n> > +\t\t\tdie(\"bad tag\");\n> \n> I haven't looked yet, but I suspect that this is as impossible as the \n> invalid parent.\n\nEg. tag pointing to an blob, but when calling parse_tag_buffer, a non\nblob object is loaded under blob sha1.\n\nBut wouldn't it be simpler, if we would add\n\tif (!item->tagged)\n\t\treturn -1;\nin parse_tag_buffer?\n\n> > diff --git a/sha1_file.c b/sha1_file.c\n> > index 4179949..c25ce64 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -1943,7 +1943,8 @@ void *read_object_with_reference(const unsigned char *sha1,\n> > \t\t}\n> > \t\tref_length = strlen(ref_type);\n> > \n> > -\t\tif (memcmp(buffer, ref_type, ref_length) ||\n> > +\t\tif (ref_length + 40 >= size ||\n> > +\t\t memcmp(buffer, ref_type, ref_length) ||\n> \n> Makes sense.\n> \n> > @@ -494,6 +498,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)\n> > \t\t\t\treturn error(\"%.*s: expected %s type, but the object dereferences to %s type\",\n> > \t\t\t\t\t len, name, typename(expected_type),\n> > \t\t\t\t\t typename(o->type));\n> > +\t\t\tif (!o)\n> > +\t\t\t\treturn -1;\n> \n> You probably want to guard for ((tag *)o)->tagged == NULL; Okay, now I am \n> curious. *megoesandlooks* Indeed, if the tag refers to an unknown type, \n> tagged = NULL.\n> \n> But I think in that case, it should return an error(). And maybe only for \n> type == OBJ_TAG.\n> \n> > @@ -580,6 +586,8 @@ static int handle_one_ref(const char *path,\n> > \t\treturn 0;\n> > \tif (object->type == OBJ_TAG)\n> > \t\tobject = deref_tag(object, path, strlen(path));\n> > +\tif (!object)\n> > +\t\treturn 0;\n> \n> As above, it looks strange to see that object->something is checked, and \n> _then_ object. I'd put this into curly brackets, together with the \n> deref_tag(), just to help the reader a bit, should she be as weak in mind \n> as yours truly.\n> \n> > @@ -617,7 +625,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n> > \t\tunsigned long size;\n> > \n> > \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n> > -\t\tparse_object(commit->object.sha1);\n> > +\t\tif(!parse_object(commit->object.sha1))\n> > +\t\t\tcontinue;\n> \n> Makes sense, but please add a space after the \"if\".\n> \n> > diff --git a/tag.c b/tag.c\n> > index 38bf913..990134f 100644\n> > --- a/tag.c\n> > +++ b/tag.c\n> > @@ -9,7 +9,10 @@ const char *tag_type = \"tag\";\n> > struct object *deref_tag(struct object *o, const char *warn, int warnlen)\n> > {\n> > \twhile (o && o->type == OBJ_TAG)\n> > -\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n> > +\t\tif (((struct tag *)o)->tagged)\n> > +\t\t\to = parse_object(((struct tag *)o)->tagged->sha1);\n> > +\t\telse\n> > +\t\t\to = NULL;\n> \n> Knowing that tagged _can_ be NULL, this makes tons of sense. Except that \n> again, I'd call error() to tell the user, before setting o = NULL.\n\nmfg Martin Kögler\n"}]}