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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2008, 07:37 UTC
Message-ID: <7vwsotmqq2.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080225072658.GB15761@auto.tuwien.ac.at>

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

Show 23 quoted lines

> On Sun, Feb 24, 2008 at 10:04:04PM -0500, Shawn O. Pearce wrote:
>> Martin Koegler <mkoegler@auto.tuwien.ac.at> wrote:
>> Hmm.  Don't you need to get commit->parenst *after* it is parsed,
>> and not before?
>
> Thanks, fixed.
>
>> > @@ -0,0 +1,10 @@
>> > +#ifndef GIT_FSCK_H
>> > +#define GIT_FSCK_H
>> > +
>> > +#define OBJ_ANY OBJ_BAD
>> 
>> Its unclear why this macro is necessary.
>
> There are cases (eg. a tag->tagged), where the object type does not
> matter. In this case, the callback gets this value for the expected
> type.
>
> The value of OBJ_BAD (-1) is not used anywhere in git, so its
> free. OBJ_ANY is just a better name.
>
> If somebody has a better idea, please tell me.

As I mentioned in my previous review comment, the token is used to tell the walker callback function that this type of object was expected where it was found, in order to allow the callback to check the type of the object. I think OBJ_ANY is a very good name for the token you would use to tell the callback that any type of object can appear (because that is a taggee).

So I do not have objection to OBJ_ANY (but again, this kind of thing needs to be explained in your commit log message), but giving the token the same value as OBJ_BAD may not be such a cool idea. After all, if the walker callback was told with OBJ_ANY that any type of object is Ok, it should still say "oops" if the given object said it actually is of type OBJ_BAD. E.g. in your [2/4] patch:

        +static int mark_object(struct object *obj, int type, void *data)
        +{
        + ...
        +	if (type != OBJ_ANY && obj->type != type) {
        +		objerror(parent, "wrong object type in link");
        +	}

if you use the above #define, a tagged object that has a bad type will pass this check unnoticed, won't it?