| Message ID | patch-01.19-a74e02ff0ba-20210331T190531Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tree-walk.h: slimmed down | expand |
Æ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.
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. + */ 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) */ } /* Double-check local_repo_env below if you add to this list. */
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. 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- cache.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)