Re: [PATCH 1/4] add generic, type aware object chain walker
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 25, 2008, 03:08 UTC
- Message-ID
- <7vr6f1pwaw.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <12038642373342-git-send-email-mkoegler@auto.tuwien.ac.at>
Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
Show 10 quoted lines
> diff --git a/Makefile b/Makefile > index a9b5a67..3b356f8 100644 > --- a/Makefile > +++ b/Makefile > @@ -303,7 +303,7 @@ LIB_H = \ > run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \ > tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \ > utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \ > - mailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h > + mailmap.h remote.h parse-options.h transport.h diffcore.h hash.h ll-merge.h fsck.h
I'd rather see a series does not depend on things in next that you do not have to depend on, pretty please?
The patches have style issues everywhere. The opening paren that surrounds the conditional to if/while should always have a SP before it, while function names at the function callsite should immediately be followed by an open paren.
Show 23 quoted lines
> +static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
> +{
> + struct tree_desc desc;
> + struct name_entry entry;
> +
> + if(parse_tree(tree))
> + return -1;
> +
> + init_tree_desc(&desc, tree->buffer, tree->size);
> + while(tree_entry(&desc, &entry)) {
> + int result;
> +
> + if (S_ISGITLINK(entry.mode))
> + continue;
> + if (S_ISDIR(entry.mode))
> + result = walk(&lookup_tree(entry.sha1)->object, OBJ_TREE, data);
> + else if (S_ISREG(entry.mode) || S_ISLNK(entry.mode))
> + result = walk(&lookup_blob(entry.sha1)->object, OBJ_BLOB, data);
> + else {
> + error("in tree %s: entry %s has bad mode %.6o\n",
> + sha1_to_hex(tree->object.sha1), entry.path, entry.mode);
> + result = -1;
> + }Would "result = error(...)" be too cute ;-)?
Show 20 quoted lines
> +static int fsck_walk_commit(struct commit *commit, fsck_walk_func walk, void *data)
> +{
> + struct commit_list *parents = commit->parents;
> + int result;
> +
> + if(parse_commit(commit))
> + return -1;
> +
> + result = walk((struct object*)commit->tree, OBJ_TREE, data);
> + if (result)
> + return result;
> +
> + while (parents) {
> + result = walk((struct object*)parents->item, OBJ_COMMIT, data);
> + if (result)
> + return result;
> + parents = parents->next;
> + }
> + return 0;
> +}Hmm. For the purpose of proving there is _no_ error (or an error or more), it would be Ok to return early like this, but won't there be cases where you would want to get as many coverage as possible?
For example, I do not think you can use this to mark reachable objects. Even if you find error walking the first parent history, you would want to still mark a healthy second parent history reachable.
Show 12 quoted lines
> diff --git a/fsck.h b/fsck.h > new file mode 100644 > index 0000000..fccc89f > --- /dev/null > +++ b/fsck.h > @@ -0,0 +1,10 @@ > +#ifndef GIT_FSCK_H > +#define GIT_FSCK_H > + > +#define OBJ_ANY OBJ_BAD > + > +typedef int (*fsck_walk_func)(struct object *obj, int type, void *data);
It is unclear in this patch, but this "type" is not telling what the type of the object is to the walk_func, but is telling it that the given object was found in a place where object of that type is expected/required, so that walk_func can check and complain, right? Needs a bit of commenting...