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

Martin Koegler [off-list ref] 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.