Re: [PATCH 1/4] add generic, type aware object chain walker
- From
- Martin Koegler <mkoegler@auto.tuwien.ac.at>
- Date
- Feb 25, 2008, 07:46 UTC
- Message-ID
- <20080225074611.GC15761@auto.tuwien.ac.at>
- In-Reply-To
- <7vr6f1pwaw.fsf@gitster.siamese.dyndns.org>
On Sun, Feb 24, 2008 at 07:08:39PM -0800, Junio C Hamano wrote:
Show 13 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?
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?
Show 30 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.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