On Sun, 12 May 2024 14:37:23 GMT, Chen Liang <li...@openjdk.org> wrote:

> I believe this field only captures the 2 bytes at higher indices of External 
> File Attribute; it's not the complete 4-byte external file attributes and 
> this value is not captured unless "Version made by" field's larger index byte 
> (byte 5) is 3. So this name may still be imperfect.

The purpose of this PR was mainly to reduce the risk of confusion with the 
"extra field", which is an entirely unrelated concept.

While I agree that the name `externalFileAttributes` might not express the full 
semantics of the field perfectly, I'm honestly not sure which name would, given 
that `APPNOTE.TXT` is pretty opaque in describing what these two bytes express, 
and as you point out, the field only carries data when "Version Made By" 
indicates Unix.

Instead of trying to find name which covers the full semantics, I suggest that 
we keep `externalFileAttributes` and instead seek to improve the comments 
relevant to this field:

* The field comment currently says `// File type, setuid, setgid, sticky, POSIX 
permissions`, we could prepend something clarifying that the field only carries 
data for Unix entries.
* Line 699 says `// read all bits in this field, including sym link 
attributes`, this could be improved to clarify that only the higher two 
"unix-bytes" are read ("all bits in this field" is a bit unclear now)
* Line 700 masks off the Unix bytes with `0xFFFF`, this isn't strictly 
necessary since `CENATX_PERMS` already only reads the two bytes we want. 
Perhaps clearer just to drop this masking.
* `ZipUtils.CENATX_PERMS` reads the full 16 bits of the Unix external file 
attributes, not just the permissions. The current name is confusing.

Since I don't want to delay the integration of the this PR any further, I 
suggest we tackle the above clarifications in a follow-up PR. Would that be ok 
with you, @liach ?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2198283812

Reply via email to