Re: [PATCH 1/4] add generic, type aware object chain walker

[Junio C Hamano <gitster@pobox.com>]

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> 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.

> +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 ;-)?

> +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.

> 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...