{"thread":{"id":"12293","subject":"[PATCH 1/4] add generic, type aware object chain walker","startedAt":"2008-02-24T14:43:54Z","lastAt":"2008-02-25T17:49:23Z","messageCount":19,"participants":["Martin Koegler","Shawn O. Pearce","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"69790","messageId":"12038642373342-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12293","inReplyTo":null,"subject":"[PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-24T14:43:54Z","receivedAt":"2008-02-24T14:43:54Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"The 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 <mkoegler@auto.tuwien.ac.at>\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 a9b5a67..3b356f8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -303,7 +303,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 ll-merge.h\n+\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h fsck.h\n \n DIFF_OBJS = \\\n \tdiff.o diff-lib.o diffcore-break.o diffcore-order.o \\\n@@ -327,7 +327,7 @@ LIB_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 branch.o \\\n-\tll-merge.o\n+\tll-merge.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..92254fb\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..fccc89f\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.2.gf624.dirty\n"},{"id":"69791","messageId":"1203864237774-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"12038642373342-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 2/4] builtin-fsck: move away from object-refs","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-24T14:43:55Z","receivedAt":"2008-02-24T14:43:55Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\nreachable.c is not the optimal thing for checking a repository.\nSome problems are ignored by it while it bails out on other errors.\n\nfsck should not bail out at the first error (well, there are some\ndie in tree-walk.c), so the fsck_walk callbacks return 0, even in\nthe case of an error. The error is propagated via errors_found.\n\nThe patch is slightly tested.\n\n builtin-fsck.c |   95 +++++++++++++++++++++++++++++++++++++++----------------\n 1 files changed, 67 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex cc7524b..512346a 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@@ -63,13 +64,70 @@ static int objwarning(struct object *obj, const char *err, ...)\n \treturn -1;\n }\n \n+static int mark_object(struct object *obj, int type, void *data)\n+{\n+\tstruct tree *tree = NULL;\n+\tstruct object *parent = data;\n+\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}\n+\n+\tif (obj->flags & REACHABLE)\n+\t\treturn 0;\n+\tobj->flags |= REACHABLE;\n+\tif (!obj->parsed) {\n+\t\tif (parent && !has_sha1_file(obj->sha1)) {\n+\t\t\tprintf(\"broken link from %7s %s\\n\",\n+\t\t\t\t typename(parent->type), sha1_to_hex(parent->sha1));\n+\t\t\tprintf(\"              to %7s %s\\n\",\n+\t\t\t\t typename(obj->type), sha1_to_hex(obj->sha1));\n+\t\t\terrors_found |= ERROR_REACHABLE;\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n+\tif (obj->type == OBJ_TREE) {\n+\t\tobj->parsed = 0;\n+\t\ttree = (struct tree*)obj;\n+\t\tif (parse_tree(tree) < 0)\n+\t\t\treturn 0; /* error already displayed */\n+\t}\n+\tfsck_walk(obj, mark_object, obj);\n+\tif (tree) {\n+\t\tfree(tree->buffer);\n+\t\ttree->buffer=NULL;\n+\t}\n+\treturn 0;\n+}\n+\n+static void mark_object_reachable(struct object *obj)\n+{\n+\tmark_object(obj, OBJ_ANY, 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 /*\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@@ -82,25 +140,6 @@ static void check_reachable_object(struct object *obj)\n \t\terrors_found |= ERROR_REACHABLE;\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 }\n \n /*\n@@ -414,6 +453,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@@ -538,13 +578,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 +614,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 +700,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@@ -693,7 +733,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@@ -741,7 +780,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 +812,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.2.gf624.dirty\n"},{"id":"69792","messageId":"1203864237817-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"1203864237774-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 3/4] Remove unused object-ref code","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-24T14:43:56Z","receivedAt":"2008-02-24T14:43:56Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\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 3b356f8..a184f6e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -318,7 +318,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 7dff653..d9f7db8 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2012,7 +2012,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 6f7d5f8..921113f 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -607,7 +607,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 22ce776..6684c4e 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 990134f..4470d2b 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -87,12 +87,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 b26d053..e5421db 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -393,7 +393,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.2.gf624.dirty\n"},{"id":"69793","messageId":"12038642372833-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"1203864237817-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"[PATCH 4/4] builtin-fsck: reports missing parent commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-24T14:43:57Z","receivedAt":"2008-02-24T14:43:57Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n builtin-fsck.c |   24 ++++++++++++++++++++++++\n commit.c       |    2 +-\n commit.h       |    1 +\n 3 files changed, 26 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 512346a..18a660f 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -394,6 +394,8 @@ static int fsck_commit(struct commit *commit)\n {\n \tchar *buffer = commit->buffer;\n \tunsigned char tree_sha1[20], sha1[20];\n+\tstruct commit_graft *graft;\n+\tint parents = 0;\n \n \tif (verbose)\n \t\tfprintf(stderr, \"Checking commit %s\\n\",\n@@ -411,6 +413,28 @@ static int fsck_commit(struct commit *commit)\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\tparents++;\n+\t}\n+\tgraft = lookup_commit_graft(commit->object.sha1);\n+\tif (graft) {\n+\t\tstruct commit_list *p = commit->parents;\n+\t\tparents = 0;\n+\t\twhile (p && parents) {\n+\t\t\tp = p->next;\n+\t\t\tparents -- ;\n+\t\t}\n+\t\tif (graft->nr_parent == -1 && !parents)\n+\t\t\t; /* shallow commit */\n+\t\telse if (graft->nr_parent != parents)\n+\t\t\treturn objerror(&commit->object, \"graft objects missing\");\n+\t} else {\n+\t\tstruct commit_list *p = commit->parents;\n+\t\twhile (p && parents) {\n+\t\t\tp = p->next;\n+\t\t\tparents -- ;\n+\t\t}\n+\t\tif (p || parents)\n+\t\t\treturn objerror(&commit->object, \"parent objects missing\");\n \t}\n \tif (memcmp(buffer, \"author \", 7))\n \t\treturn objerror(&commit->object, \"invalid format - expected 'author' line\");\ndiff --git a/commit.c b/commit.c\nindex 6684c4e..94d5b3d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -193,7 +193,7 @@ static void prepare_commit_graft(void)\n \tcommit_graft_prepared = 1;\n }\n \n-static struct commit_graft *lookup_commit_graft(const unsigned char *sha1)\n+struct commit_graft *lookup_commit_graft(const unsigned char *sha1)\n {\n \tint pos;\n \tprepare_commit_graft();\ndiff --git a/commit.h b/commit.h\nindex 80d65b9..a1e9591 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -116,6 +116,7 @@ struct commit_graft {\n struct commit_graft *read_graft_line(char *buf, int len);\n int register_commit_graft(struct commit_graft *, int);\n int read_graft_file(const char *graft_file);\n+struct commit_graft *lookup_commit_graft(const unsigned char *sha1);\n \n extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);\n \n-- \n1.5.4.2.gf624.dirty\n"},{"id":"69841","messageId":"20080225030404.GL8410@spearce.org","threadId":"12293","inReplyTo":"12038642373342-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-25T03:04:04Z","receivedAt":"2008-02-25T03:04:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\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\nHmm.  Don't you need to get commit->parenst *after* it is parsed,\nand not before?\n\n> @@ -0,0 +1,10 @@\n> +#ifndef GIT_FSCK_H\n> +#define GIT_FSCK_H\n> +\n> +#define OBJ_ANY OBJ_BAD\n\nIts unclear why this macro is necessary.\n\n-- \nShawn.\n"},{"id":"69843","messageId":"7vr6f1pwaw.fsf@gitster.siamese.dyndns.org","threadId":"12293","inReplyTo":"12038642373342-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T03:08:39Z","receivedAt":"2008-02-25T03:08:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> diff --git a/Makefile b/Makefile\n> index a9b5a67..3b356f8 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -303,7 +303,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 ll-merge.h\n> +\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h fsck.h\n\nI'd rather see a series does not depend on things in next that\nyou do not have to depend on, pretty please?\n\nThe patches have style issues everywhere.  The opening paren\nthat surrounds the conditional to if/while should always have a\nSP before it, while function names at the function callsite\nshould immediately be followed by an open paren.\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\nWould \"result = error(...)\" be too cute ;-)?\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\nHmm.  For the purpose of proving there is _no_ error (or an\nerror or more), it would be Ok to return early like this, but\nwon't there be cases where you would want to get as many\ncoverage as possible?\n\nFor example, I do not think you can use this to mark reachable\nobjects.  Even if you find error walking the first parent\nhistory, you would want to still mark a healthy second parent\nhistory reachable.\n\n> diff --git a/fsck.h b/fsck.h\n> new file mode 100644\n> index 0000000..fccc89f\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\nIt is unclear in this patch, but this \"type\" is not telling what\nthe type of the object is to the walk_func, but is telling it\nthat the given object was found in a place where object of that\ntype is expected/required, so that walk_func can check and\ncomplain, right?  Needs a bit of commenting...\n"},{"id":"293732","messageId":"20080225072658.GB15761@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"20080225030404.GL8410@spearce.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T07:26:58Z","receivedAt":"2008-02-25T07:26:58Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 24, 2008 at 10:04:04PM -0500, Shawn O. Pearce wrote:\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n> Hmm.  Don't you need to get commit->parenst *after* it is parsed,\n> and not before?\n\nThanks, fixed.\n\n> > @@ -0,0 +1,10 @@\n> > +#ifndef GIT_FSCK_H\n> > +#define GIT_FSCK_H\n> > +\n> > +#define OBJ_ANY OBJ_BAD\n> \n> Its unclear why this macro is necessary.\n\nThere are cases (eg. a tag->tagged), where the object type does not\nmatter. In this case, the callback gets this value for the expected\ntype.\n\nThe value of OBJ_BAD (-1) is not used anywhere in git, so its\nfree. OBJ_ANY is just a better name.\n\nIf somebody has a better idea, please tell me.\n\n"},{"id":"69857","messageId":"7vwsotmqq2.fsf@gitster.siamese.dyndns.org","threadId":"12293","inReplyTo":"20080225072658.GB15761@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T07:37:25Z","receivedAt":"2008-02-25T07:37:25Z","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> On Sun, Feb 24, 2008 at 10:04:04PM -0500, Shawn O. Pearce wrote:\n>> Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n>> Hmm.  Don't you need to get commit->parenst *after* it is parsed,\n>> and not before?\n>\n> Thanks, fixed.\n>\n>> > @@ -0,0 +1,10 @@\n>> > +#ifndef GIT_FSCK_H\n>> > +#define GIT_FSCK_H\n>> > +\n>> > +#define OBJ_ANY OBJ_BAD\n>> \n>> Its unclear why this macro is necessary.\n>\n> There are cases (eg. a tag->tagged), where the object type does not\n> matter. In this case, the callback gets this value for the expected\n> type.\n>\n> The value of OBJ_BAD (-1) is not used anywhere in git, so its\n> free. OBJ_ANY is just a better name.\n>\n> If somebody has a better idea, please tell me.\n\nAs I mentioned in my previous review comment, the token is used\nto tell the walker callback function that this type of object\nwas expected where it was found, in order to allow the callback\nto check the type of the object.  I think OBJ_ANY is a very good\nname for the token you would use to tell the callback that any\ntype of object can appear (because that is a taggee).\n\nSo I do not have objection to OBJ_ANY (but again, this kind of\nthing needs to be explained in your commit log message), but\ngiving the token the same value as OBJ_BAD may not be such a\ncool idea.  After all, if the walker callback was told with\nOBJ_ANY that any type of object is Ok, it should still say\n\"oops\" if the given object said it actually is of type OBJ_BAD.\nE.g. in your [2/4] patch:\n\n        +static int mark_object(struct object *obj, int type, void *data)\n        +{\n        + ...\n        +\tif (type != OBJ_ANY && obj->type != type) {\n        +\t\tobjerror(parent, \"wrong object type in link\");\n        +\t}\n\nif you use the above #define, a tagged object that has a bad\ntype will pass this check unnoticed, won't it?\n"},{"id":"69858","messageId":"20080225074611.GC15761@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"7vr6f1pwaw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T07:46:11Z","receivedAt":"2008-02-25T07:46:11Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 24, 2008 at 07:08:39PM -0800, Junio C Hamano wrote:\n> > diff --git a/Makefile b/Makefile\n> > index a9b5a67..3b356f8 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -303,7 +303,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 ll-merge.h\n> > +\tmailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h fsck.h\n> \n> I'd rather see a series does not depend on things in next that\n> you do not have to depend on, pretty please?\n\nI usually develop my patch on next. I can offer you two things:\n* base my patches on something different (master?)\n* add fsck.h/o some lines above\n\nWhat do you prefer?\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> Hmm.  For the purpose of proving there is _no_ error (or an\n> error or more), it would be Ok to return early like this, but\n> won't there be cases where you would want to get as many\n> coverage as possible?\n> \n> For example, I do not think you can use this to mark reachable\n> objects.  Even if you find error walking the first parent\n> history, you would want to still mark a healthy second parent\n> history reachable.\n\nHow should I define the return value of fsck_walk in the presence of\nmultiple errors?\n\nIt would not be necessary for all my users:\n\n* in unpack-object and index-pack (I'll send an updated patch in the\n  next days), any error means that we can abort. Further checking would\n  mean wasting of resources.\n\n* in fsck (patch 2) the error is signaled by the errors_found variable, so\n  all callbacks can return 0, even in the case of an error. Checking the\n  return value of fsck_walk would mean duplicate error messages.\n\nmfg Martin Kögler\n"},{"id":"69859","messageId":"20080225075242.GD15761@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"7vwsotmqq2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T07:52:42Z","receivedAt":"2008-02-25T07:52:42Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 24, 2008 at 11:37:25PM -0800, Junio C Hamano wrote:\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> So I do not have objection to OBJ_ANY (but again, this kind of\n> thing needs to be explained in your commit log message), but\n> giving the token the same value as OBJ_BAD may not be such a\n> cool idea.  \n\nWhat about\n#define OBJ_BAD -2\n\n> After all, if the walker callback was told with\n> OBJ_ANY that any type of object is Ok, it should still say\n> \"oops\" if the given object said it actually is of type OBJ_BAD.\n> E.g. in your [2/4] patch:\n> \n>         +static int mark_object(struct object *obj, int type, void *data)\n>         +{\n>         + ...\n>         +\tif (type != OBJ_ANY && obj->type != type) {\n>         +\t\tobjerror(parent, \"wrong object type in link\");\n>         +\t}\n> \n> if you use the above #define, a tagged object that has a bad\n> type will pass this check unnoticed, won't it?\n\nNo, it wouldn't, as object->type is never initialized to OBJ_BAD:\n$ grep \"OBJ_BAD\" *.c *.h\ncache.h:        OBJ_BAD = -1,\n\nmfg Martin Kögler\n"},{"id":"69860","messageId":"7vskzhmppo.fsf@gitster.siamese.dyndns.org","threadId":"12293","inReplyTo":"20080225074611.GC15761@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T07:59:15Z","receivedAt":"2008-02-25T07:59:15Z","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> On Sun, Feb 24, 2008 at 07:08:39PM -0800, Junio C Hamano wrote:\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>> Hmm.  For the purpose of proving there is _no_ error (or an\n>> error or more), it would be Ok to return early like this, but\n>> won't there be cases where you would want to get as many\n>> coverage as possible?\n>> \n>> For example, I do not think you can use this to mark reachable\n>> objects.  Even if you find error walking the first parent\n>> history, you would want to still mark a healthy second parent\n>> history reachable.\n>\n> How should I define the return value of fsck_walk in the presence of\n> multiple errors?\n\nReturning error is fine.  That is not what I was talking about.\n\nI was talking about an early return in the code, that does not\ncallback once you find an error.\n"},{"id":"69861","messageId":"7vmyppmpko.fsf@gitster.siamese.dyndns.org","threadId":"12293","inReplyTo":"20080225075242.GD15761@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-25T08:02:15Z","receivedAt":"2008-02-25T08:02:15Z","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> What about\n> #define OBJ_BAD -2\n\nYou mean \"#define OBJ_ANY -2\"?\n\n>> After all, if the walker callback was told with\n>> OBJ_ANY that any type of object is Ok, it should still say\n>> \"oops\" if the given object said it actually is of type OBJ_BAD.\n>> E.g. in your [2/4] patch:\n>> \n>>         +static int mark_object(struct object *obj, int type, void *data)\n>>         +{\n>>         + ...\n>>         +\tif (type != OBJ_ANY && obj->type != type) {\n>>         +\t\tobjerror(parent, \"wrong object type in link\");\n>>         +\t}\n>> \n>> if you use the above #define, a tagged object that has a bad\n>> type will pass this check unnoticed, won't it?\n>\n> No, it wouldn't, as object->type is never initialized to OBJ_BAD:\n> $ grep \"OBJ_BAD\" *.c *.h\n> cache.h:        OBJ_BAD = -1,\n\nDon't you think that is too subtle?  Don't you want to future\nproof your code?\n\nUsing the OBJ_ANY (which I'd agree is a good name) that is\ndifferent from anything else that is already defined would be a\ngood idea for that, I would think.\n"},{"id":"69862","messageId":"20080225080445.GM8410@spearce.org","threadId":"12293","inReplyTo":"20080225075242.GD15761@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-25T08:04:45Z","receivedAt":"2008-02-25T08:04:45Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n> \n> What about\n> #define OBJ_BAD -2\n\nAll of the other OBJ_* constants are declared in the enum\nobject_type, not as #defines.\n\nSee below for why this should not be <0.\n \n> > After all, if the walker callback was told with\n> > OBJ_ANY that any type of object is Ok, it should still say\n> > \"oops\" if the given object said it actually is of type OBJ_BAD.\n> > E.g. in your [2/4] patch:\n> > \n> >         +static int mark_object(struct object *obj, int type, void *data)\n> >         +{\n> >         + ...\n> >         +\tif (type != OBJ_ANY && obj->type != type) {\n> >         +\t\tobjerror(parent, \"wrong object type in link\");\n> >         +\t}\n> > \n> > if you use the above #define, a tagged object that has a bad\n> > type will pass this check unnoticed, won't it?\n> \n> No, it wouldn't, as object->type is never initialized to OBJ_BAD:\n> $ grep \"OBJ_BAD\" *.c *.h\n> cache.h:        OBJ_BAD = -1,\n\nToday that may be true.  OBJ_BAD was created for cases where the\ninternal object header parsing code in say sha1_file.c finds a type\ncode it doesn't recognize and wants to return it back to the caller.\nThis grew out of some of the early pack v4 work.\n\nIt actually happens today when a function that returns an object\ntype enum returns failure.  sha1_object_info() is one such function.\nIt returns \"int\" but that int is also really an enum object_type.\nWhen that function fails it returns -1, which has it happens\nis OBJ_BAD.\n\nSubtle, yes.  But OBJ_BAD has meaning and is in use today.  Please do\nnot use it for \"we don't care what it is\".\n\n-- \nShawn.\n"},{"id":"69863","messageId":"20080225080653.GA20048@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"7vmyppmpko.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T08:06:53Z","receivedAt":"2008-02-25T08:06:53Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Mon, Feb 25, 2008 at 12:02:15AM -0800, Junio C Hamano wrote:\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> \n> > What about\n> > #define OBJ_BAD -2\n> \n> You mean \"#define OBJ_ANY -2\"?\n\nYes.\n\nShould it go into cache.h or should it stay in fsck.h?\n\nmfg Martin Kögler\n"},{"id":"69864","messageId":"20080225081028.GN8410@spearce.org","threadId":"12293","inReplyTo":"20080225074611.GC15761@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-25T08:10:28Z","receivedAt":"2008-02-25T08:10:28Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n> On Sun, Feb 24, 2008 at 07:08:39PM -0800, Junio C Hamano wrote:\n> > \n> > I'd rather see a series does not depend on things in next that\n> > you do not have to depend on, pretty please?\n> \n> I usually develop my patch on next. I can offer you two things:\n> * base my patches on something different (master?)\n> * add fsck.h/o some lines above\n> \n> What do you prefer?\n\nDon't build patch series against \"next\" unless they will trivially\napply *and function correctly* also on \"master\".  E.g. a trivially\nobvious one liner may be OK to develop on \"next\", but everything\nelse should be done against \"master\", or if you must the tip of the\ntopic in \"next\" that you desire to build upon.\n\nE.g. when I was working on the builtin-fetch debugging and I was\nbuilding on top of that topic in next, not all of next itself.\n \n> > For example, I do not think you can use this to mark reachable\n> > objects.  Even if you find error walking the first parent\n> > history, you would want to still mark a healthy second parent\n> > history reachable.\n> \n> How should I define the return value of fsck_walk in the presence of\n> multiple errors?\n> \n> It would not be necessary for all my users:\n> \n> * in unpack-object and index-pack (I'll send an updated patch in the\n>   next days), any error means that we can abort. Further checking would\n>   mean wasting of resources.\n> \n> * in fsck (patch 2) the error is signaled by the errors_found variable, so\n>   all callbacks can return 0, even in the case of an error. Checking the\n>   return value of fsck_walk would mean duplicate error messages.\n\nSo the return value is more about \"keep walking\" than it is about an\nerror (or lack there of)?\n\n-- \nShawn.\n"},{"id":"69865","messageId":"20080225081206.GO8410@spearce.org","threadId":"12293","inReplyTo":"20080225080653.GA20048@auto.tuwien.ac.at","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-25T08:12:06Z","receivedAt":"2008-02-25T08:12:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n> On Mon, Feb 25, 2008 at 12:02:15AM -0800, Junio C Hamano wrote:\n> > mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > \n> > > What about\n> > > #define OBJ_BAD -2\n> > \n> > You mean \"#define OBJ_ANY -2\"?\n> \n> Yes.\n> \n> Should it go into cache.h or should it stay in fsck.h?\n\nI'd rather see this declared at the end of enum object_type, as\nessentially OBJ_MAX-1.  But Nico may have a different opinion.\n\nAs such it shouldn't need a numerical constant.  We don't encode\nthis constant into packfiles.\n\n-- \nShawn.\n"},{"id":"69866","messageId":"20080225082145.GB20048@auto.tuwien.ac.at","threadId":"12293","inReplyTo":"7vskzhmppo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-02-25T08:21:45Z","receivedAt":"2008-02-25T08:21:45Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sun, Feb 24, 2008 at 11:59:15PM -0800, Junio C Hamano wrote:\n> mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > How should I define the return value of fsck_walk in the presence of\n> > multiple errors?\n> \n> Returning error is fine.  That is not what I was talking about.\n> \n> I was talking about an early return in the code, that does not\n> callback once you find an error.\n\nThat is not what I was talking about. \n\nWe need to define the return code of fsck_walk in the case of multiple\nerrors. What error (of all possible) should it return?\n\nWhat about this new behaviour:\n\nCallback return values:\n* 0 everything OK\n* > 0 error, but continue\n* < 0 error, but stop\n\nThe return value of fsck_walk would be:\n* -1, if fsck_walk discoveres an error (eg. not parseable object)\n* first callback return value < 0 (and stops further processing in this case)\n* first callback return value > 0 (if not callback return value < 0)\n* 0, if no errors are discovered\n\nmfg Martin Kögler\n"},{"id":"69893","messageId":"alpine.LFD.1.00.0802251233020.3167@xanadu.home","threadId":"12293","inReplyTo":"20080225081206.GO8410@spearce.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-25T17:35:31Z","receivedAt":"2008-02-25T17:35:31Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 25 Feb 2008, Shawn O. Pearce wrote:\n\n> Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:\n> > On Mon, Feb 25, 2008 at 12:02:15AM -0800, Junio C Hamano wrote:\n> > > mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:\n> > > \n> > > > What about\n> > > > #define OBJ_BAD -2\n> > > \n> > > You mean \"#define OBJ_ANY -2\"?\n> > \n> > Yes.\n> > \n> > Should it go into cache.h or should it stay in fsck.h?\n> \n> I'd rather see this declared at the end of enum object_type, as\n> essentially OBJ_MAX-1.  But Nico may have a different opinion.\n\nWhy not alias OBJ_ANY to OBJ_NONE instead?\n\n\nNicolas\n"},{"id":"69894","messageId":"alpine.LFD.1.00.0802251235510.3167@xanadu.home","threadId":"12293","inReplyTo":"20080225080445.GM8410@spearce.org","subject":"Re: [PATCH 1/4] add generic, type aware object chain walker","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-02-25T17:49:23Z","receivedAt":"2008-02-25T17:49:23Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 25 Feb 2008, Shawn O. Pearce wrote:\n\n> > No, it wouldn't, as object->type is never initialized to OBJ_BAD:\n> > $ grep \"OBJ_BAD\" *.c *.h\n> > cache.h:        OBJ_BAD = -1,\n> \n> Today that may be true.  OBJ_BAD was created for cases where the\n> internal object header parsing code in say sha1_file.c finds a type\n> code it doesn't recognize and wants to return it back to the caller.\n> This grew out of some of the early pack v4 work.\n> \n> It actually happens today when a function that returns an object\n> type enum returns failure.  sha1_object_info() is one such function.\n> It returns \"int\" but that int is also really an enum object_type.\n> When that function fails it returns -1, which has it happens\n> is OBJ_BAD.\n> \n> Subtle, yes.  But OBJ_BAD has meaning and is in use today.  Please do\n> not use it for \"we don't care what it is\".\n\nActually, the reason for OBJ_BAD is possibly even more subtle than that.  \nIt is \"documented\" in commit fef742c4ed though.\n\nThe idea is for some function returning an object type to also be able \nto return an error condition without the overhead and/or hassle of a \nseparate argument.  So the object space is returned as a positive value, \nand error space is negative.  The only purpose of OBJ_BAD was to make \nsure object_type is a signed type in order to avoid issues with \ncompilers which might interpret the C standard in a way that would \nreduce the enum to its minimal expression such as some \"signedlessness\".\n\n\nNicolas\n"}]}