On Mon, Dec 1, 2014 at 7:48 AM, David Michael <fedora.dm0@xxxxxxxxx> wrote:
> On Mon, Dec 1, 2014 at 12:55 AM, Torsten Bögershausen <tboegi@xxxxxx> wrote:
>> On 12/01/2014 04:40 AM, David Michael wrote:
>>>
>>> On Sun, Nov 30, 2014 at 3:16 PM, Torsten Bögershausen <tboegi@xxxxxx>
>>> wrote:
>>> [snip]
>>>>
>>>> Could the code be more human-readable ?
>>>> static inline mode_t mode_native_to_git(mode_t native_mode)
>>>> {
>>>> int perm_bits = native_mode & 07777;
>>>> if (S_ISREG(native_mode))
>>>> return 0100000 | perm_bits;
>>>> if (S_ISDIR(native_mode))
>>>> return 0040000 | perm_bits;
>>>> if (S_ISLNK(native_mode))
>>>> return 0120000 | perm_bits;
>>>> if (S_ISBLK(native_mode))
>>>> return 0060000 | perm_bits;
>>>> if (S_ISCHR(native_mode))
>>>> return 0020000 | perm_bits;
>>>> if (S_ISFIFO(native_mode))
>>>> return 0010000 | perm_bits;
>>>> /* Non-standard type bits were given. */
>>>> /* Shouldn't we die() here ?? */
>>>> return perm_bits;
>>>> }
>>>
>>> Sure, I can send an updated patch with the new variable and without the
>>> "else"s.
>>>
>>> Regarding the question in the last comment: I was assuming if this
>>> case was ever reached, Git would handle the returned mode the same way
>>> as if it encountered an unknown/non-standard file type on a normal
>>> operating system, which could die() if needed in the function that
>>> called stat().
>>>
>>> Should I send an updated patch that die()s there?
>>>
>>> David
>>
>> Not yet, please wait with a V2 patch until I finished my thinking ;-)
>>
>> I take back the suggestion with the die(). I was thinking how to handle
>> unforeseen types, which may show up on the z/OS some day,
>> So die() is not a good idea, it is better to ignore them, what the code
>> does.
>>
>> Knowing that Git does not track block devices, nor character devices nor
>> sockets,
>> the above code could be simplyfied even more, by mapping everything which
>> is not a directory, a file or a softlink to "device type 0)
>>
>> This is just a suggestion, I want to here from others as well:
>>
>> int perm_bits = native_mode & 07777;
>> if (S_ISREG(native_mode))
>> return 0100000 | perm_bits;
>> if (S_ISDIR(native_mode))
>> return 0040000 | perm_bits;
>> if (S_ISLNK(native_mode))
>> return 0120000 | perm_bits;
>> /* Git does not track S_IFCHR, S_IFBLK, S_IFIFO, S_IFSOCK */
>> return perm_bits;
>
> I had considered omitting those three as well at first, but in this
> case it would mean that they will be unusable all together.
>
> The z/OS S_IFMT definition (i.e. the file type bit mask) is
> 0xFF000000, and the common/translated S_IFMT definition is 0xF000.
> Since the S_ISxxx macros use the typical ((mode & S_IFMT) == S_IFxxx)
> definition, they would never match a native z/OS mode after redefining
> S_IFMT.
>
> So translating those types isn't just for tracking files, it's to
> support any use of S_ISxxx anywhere in the code. It should be okay to
> remove any of those types if we know that Git will never need to use
> them.
Apologies, in my pre-coffee reply, I confused myself into thinking the
omitted types were going to be returned unchanged as opposed to being
changed to zero. That second paragraph is irrelevant.
But regarding the last paragraph: a quick grep for instances of using
those file types in the Git source found S_ISFIFO and S_ISSOCK in
git.c.
I just noticed that I copied the list of standard file type macros
from SUSv2, and S_IFSOCK was added after that. z/OS does implement
S_IFSOCK, so I think I should add it to the v2 patch to support the
test in git.c.
Another grep found no instances of testing for block or character
devices, so it should be okay to remove those from the patch if Git is
unlikely to use them in the future (unless we just want to provide all
7 types from http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
).
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html