Re: [PATCH v5 01/18] cache.h: add a comment to object_type()

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add a comment to the object_type() function to explain what it
> returns, and what the "mode" is in the "else" case.
>
> The object_type() function dates back to 4d1012c3709 (Fix rev-list
> when showing objects involving submodules, 2007-11-11). It's not
> immediately obvious to someone looking at its history and how it's
> come to be used.
>
> Despite what Linus noted in 4d1012c3709 (Fix rev-list when showing
> objects involving submodules, 2007-11-11) about wanting to move away
> from users of object_type() relying on S_ISLNK(mode) being true here
> we do currently rely on that.

You are misreading Linus's comment here.

The comment is not about "S_ISLNK()" specifically.  It is about
assuming that any "S_ISx()" macros that are designed to work on
stat.st_mode would work the same way for our "mode bits in tree"
(i.e. 'internal git state' in the commit message refers to this
fact).  Platforms do not have to have 100xxx to be regular files,
but we did rely on that bit assignment.

And then we invented S_ISGITLINK() that exists on nobody's
stat.st_mode and have been assuming that would not collide
with any real S_IFMT bit assignment.

All of that has been patched with NEEDS_MODE_TRANSLATION band-aid
quite some time ago, though, with d543d9c0 (compat: convert modes to
use portable file type values, 2014-12-03).

So, no, we do not quite rely on that anymore.

> If this is changed to a condition to
> only return OBJ_BLOB on S_ISREG(mode) then t4008, t4023 and t7415 will
> have failing tests.

Specifically, the comment is not about symbolic links, and nobody
who reads the comment correctly would imagine that limiting BLOB
to S_ISREG.  The comment merely was lamenting that

	"If ISDIR, then tree and otherwise blob" does not hold
	anymore since we added submodules; "If ISDIR, then tree, and
	if ISGITLINK, then commit and otherwise blob" would fix it,
	but relying on ISDIR, ISGITLINK, etc. was a mistake that we
	continue to rely on even with this fix.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

Which means that a lot of the stuff in the proposed log message is
false.  I do however think that ...

>  cache.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 57f2285bba9..41e99bd9c63 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -451,11 +451,16 @@ enum object_type {
>  	OBJ_MAX
>  };


> +/*
> + * object_type() returns an object of a type that'll appear in a tree,
> + * so no OBJ_TAG is possible. This is mostly (and dates back to)
> + * consumers of the tree-walk.h API's "mode" field.
> + */

... this comment is correct and it is a good change to clarify what
'mode' we are talking about here.

>  static inline enum object_type object_type(unsigned int mode)
>  {
>  	return S_ISDIR(mode) ? OBJ_TREE :
>  		S_ISGITLINK(mode) ? OBJ_COMMIT :
> -		OBJ_BLOB;
> +		OBJ_BLOB; /* S_ISREG(mode) || S_ISLNK(mode) */

For futureproofing, it might not be a bad idea to do this:

	S_ISDIR(mode) ? OBJ_TREE :
	S_ISGITLINK(mode) ? OBJ_COMMIT :
	(S_ISREG(mode) || S_ISLNK(mode)) ? OBJ_BLOB :
	OBJ_ERROR;

to anticipate new tree mode bits so that the need for a fix similar
to 4d1012c3 (Fix rev-list when showing objects involving submodules,
2007-11-11) is immediately noticed, but for now, code and comment
clarification would be sufficient.

The proposed log message needs rewriting, though.

Thanks.