From: Junio C Hamano <junkio@cox.net>
Date: Sat, 21 Apr 2007 18:42:15 GMT
Subject: Re: [PATCH 1/4] add mode parameter to get_sha1
Message-ID: <7vps5xtvqw.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <1177158380197-git-send-email-mkoegler@auto.tuwien.ac.at>

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

> If the mode parameter is not NULL, get_sha1 will store
> the mode of the object in it.

Most existing callers pass NULL to this.  Wouldn't it be cleaner
to have a new get_sha1_with_mode() function, and convert the
callers that care about mode to use it, like this?

int get_sha1(const char *str, unsigned char sha1[20]) {
	unsigned discard;
        return get_sha1_with_mode(str, sha1, &discard);
}

That way, your patch would be much easier to review and would
have less chance of getting it wrong.  I wonder if [2/4] can be
made less impact using a similar trick.  Most of the existing
callers that place objects in object_array do not know the mode.
Only some do.

+/* unknown mode */
+#define S_IFINVALID    0320000
+

This hunk does not belong to [1/4]; it is part of [2/4].

Typically S_IF$TYPE macros are masked with S_IFMT (0170000)
before being used, so the above value is *obviously* invalid,
but it also risks our code would treat it as a symlink, we do
the masking before comparison.  I wonder if defining it to
S_IFMT mask itself might be a safer option.