Re: [PATCH 1/4] add mode parameter to get_sha1

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

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.