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

[mkoegler@auto.tuwien.ac.at (Martin Koegler)]

On Sun, Feb 24, 2008 at 07:08:39PM -0800, Junio C Hamano wrote:
> > 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?

I usually develop my patch on next. I can offer you two things:
* base my patches on something different (master?)
* add fsck.h/o some lines above

What do you prefer?

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

How should I define the return value of fsck_walk in the presence of
multiple errors?

It would not be necessary for all my users:

* in unpack-object and index-pack (I'll send an updated patch in the
  next days), any error means that we can abort. Further checking would
  mean wasting of resources.

* in fsck (patch 2) the error is signaled by the errors_found variable, so
  all callbacks can return 0, even in the case of an error. Checking the
  return value of fsck_walk would mean duplicate error messages.

mfg Martin Kögler